Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add PyNumeroEvaluationError #2901

Merged
merged 3 commits into from
Jul 17, 2023
Merged

Conversation

Robbybp
Copy link
Contributor

@Robbybp Robbybp commented Jul 7, 2023

Fixes #2899

Partially.

Summary/Motivation:

To handle evaluation errors in PyNumero solver interfaces currently, we would have to catch AssertionErrors, which seems dangerous.

This is WIP for now as I haven't written any tests.

Changes proposed in this PR:

  • Add PyNumeroEvaluationError in a new pyomo.contrib.pynumero.exceptions module

This error is currently raised in the AmplInterface functions, at the same location the old asserts happened. We could imagine a different API where the low-level evaluation functions return some EvaluationResult object that contains error information, then it is the NLP's responsibility to raise the exception (currently the AmplInterface evaluation methods don't return anything). This would make it easier to return useful debugging information to the NLP, which has access to the Pyomo variables and constraints needed to display useful information. I've punted on trying to do anything like this for now, but don't think the current design precludes doing this in the future.

Legal Acknowledgement

By contributing to this software project, I have read the contribution guide and agree to the following terms and conditions for my contribution:

  1. I agree my contributions are submitted under the BSD license.
  2. I represent I am authorized to make the contributions and grant the license. If my employer has rights to intellectual property that includes these contributions, I represent that I have received permission to make contributions and grant the required license on behalf of that employer.

Copy link
Contributor

@michaelbynum michaelbynum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think your proposed (long-term) plan where the low-level functions return error information and the NLP classes raise the exception is a good one.

@codecov
Copy link

codecov bot commented Jul 10, 2023

Codecov Report

Patch coverage: 92.30% and no project coverage change.

Comparison is base (a0b4896) 87.45% compared to head (ef93dca) 87.45%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2901   +/-   ##
=======================================
  Coverage   87.45%   87.45%           
=======================================
  Files         770      771    +1     
  Lines       89609    89617    +8     
=======================================
+ Hits        78371    78378    +7     
- Misses      11238    11239    +1     
Flag Coverage Δ
linux 84.52% <92.30%> (+<0.01%) ⬆️
osx 74.56% <92.30%> (-0.01%) ⬇️
other 84.70% <92.30%> (+<0.01%) ⬆️
win 82.10% <92.30%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pyomo/contrib/pynumero/asl.py 84.21% <90.90%> (-0.03%) ⬇️
pyomo/contrib/pynumero/exceptions.py 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@Robbybp Robbybp changed the title [WIP] Add PyNumeroEvaluationError Add PyNumeroEvaluationError Jul 13, 2023
@Robbybp
Copy link
Contributor Author

Robbybp commented Jul 13, 2023

I think this is ready (pending any other reviews). I'm not adding a return object from the low-level evaluation functions at this point, but the tests I've added are high-level enough that they should not need to change if/when we do.

Copy link
Contributor

@mrmundt mrmundt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks good and I like the wrapping. I have one suggestion that may or may not be in your wheelhouse, so feel free to ignore.

pyomo/contrib/pynumero/asl.py Show resolved Hide resolved
@michaelbynum michaelbynum merged commit be93e57 into Pyomo:main Jul 17, 2023
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle evaluation errors in PyNumero
4 participants