-
Notifications
You must be signed in to change notification settings - Fork 109
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
Fix _masterFunc2
fail flag caching and add fail flag identification to IPOPT
#407
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #407 +/- ##
===========================================
- Coverage 74.33% 62.97% -11.36%
===========================================
Files 22 22
Lines 3300 3317 +17
===========================================
- Hits 2453 2089 -364
- Misses 847 1228 +381 ☔ View full report in Codecov by Sentry. |
Does the Windows action failure have something to do with my changes? It doesn't seem like it, but I'm not very familiar with it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good stuff. I had known that the fail flag is not passed to IPOPT correctly (in fact I think SNOPT is the only one that supports it). From my cursory read of the docs passing NaN
seems reasonable (and is in fact how many other optimizers prefer to handle failed evals.
As to the implementation, it looks good, I just left a minor comment if we can deal with the cache once instead of running the same line in multiple blocks.
As for the Windows failure, I suspect it's related to the recent release of numpy 2.0 (which broke our meson build in multiple ways, I haven't had time to investigate). See here for a similar log. I would suggest pinning numpy<2 in the env file first.
Thanks for the speedy review! I pinned the version of NumPy in the setup.py (I assume this is what you meant by env file?) to |
Windows builds do not use setuptools but instead uses conda. The env file used by GHA is here. Also fine with the patch version bump. |
Yay it worked! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking great! Thanks for adding the tests, you went beyond what I had in mind. Very elegant setup.
We should double check failure handling with other optimizers and maybe add a test, but that should be a separate PR. The changes in pyIPOPT
make sense.
Thanks! I agree failure handling should be checked, but I didn’t have an immediate good idea for how to test that. I’m sure there’s a way. |
Purpose
Some optimizers don't simultaneously call
_masterFunc
with the objective and constraint evaluation. In these cases, if the primal fails, the fail flag will only return the accurate value when the cache is not used (the case that actually calls the objective/constraint function). When the cache is used, the fail flag is always returned as0
. A similar behavior occurs with the gradient evaluation. This PR caches the fail flag to fix this edge case.Secondly, the pyIPOPT wrapper does not do anything if the
_masterFunc
returns aTrue
fail flag (1
). Even if any of the functions fail, IPOPT will just push on as if nothing happened. A CFD-based optimization case I saw with this was that it would get a mesh failure, immediately evaluate the gradients, go straight back to a mesh failure, and so on. The output file was as if nothing had failed and everything was normal. This is clearly not what should happen. I solved it by returningnp.array(np.NaN)
in the callback functions iffail == 1
.There are two changes in particular that I'd like feedback on:
_masterFunc2
calls itself to evaluate the primal first. In the current pyOptSparse implementation, if this primal fails, that failure is totally ignored. I modified it so that if the primal fails in this case, the gradient_masterFunc2
evaluation will return that failure value, even if the gradient evaluation succeeds. See lines 452--458 and 511-517 of the new code. Is this the right thing to do?np.array(np.NaN)
in whatever callback function is called. I don't see any more elegant way based on their docs. Although the shape isn't guaranteed to match the function's usual array shape, it appears to work based on my test cases. Does this seem like a reasonable way to solve this problem?P.S. should I update the patch version?
Expected time until merged
A week
Type of change
Testing
Run the tests I added. Also try running a simple IPOPT optimization case where some of the functions periodically fail. It should backtrack properly (it would just ignore the fail flag previously). For example:
The IPOPT output should look something like this (note the alpha cutback warnings, which do not appear with the current implementation):
Checklist
flake8
andblack
to make sure the Python code adheres to PEP-8 and is consistently formattedfprettify
or C/C++ code withclang-format
as applicable