-
Notifications
You must be signed in to change notification settings - Fork 122
Switch to cyipopt #425
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
Switch to cyipopt #425
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #425 +/- ##
==========================================
+ Coverage 74.98% 75.02% +0.03%
==========================================
Files 22 22
Lines 3338 3331 -7
==========================================
- Hits 2503 2499 -4
+ Misses 835 832 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
0b59cb5 to
10af64a
Compare
|
@kanekosh are you still using IPOPT? It would be nice if you can try out the changes here and see if they give similar results as before |
|
Thanks @ewu63! I tested cyipopt (both one from conda-force and pip-installed one with my pre-build IPOPT) and it works almost identical to my previous IPOPT installation (I sometimes see very minor difference as a results of different IPOPT and MUMPS versions). One minor fix: I happened to have a bad optimization problem where a constraint function call fails and IPOPT does "Cutting back alpha due to evaluation error" and goes on (and magically converges after that). When the evaluation error occurs, we used to return I believe it's the same for gradient and jacobian callbacks, so I made changes to return array of NaNs. However I haven't tested these so not 100% sure... |
|
I will cleanup the meson files (either here or in #431). Yeah since we are not going to use meson to install the python files I'll just remove most of the lines. Thanks for testing this out! Do you think you can add a test for the condition you described? I'm surprised we don't have one on eval failure but I suppose that part isn't very well tested. |
|
Sounds good! Just added a few tests. |
This is fine, I only know SNOPT and IPOPT support failed evaluations so I'm not surprised some of the others failed. Left a few small comments but otherwise this looks good to go. |
marcomangano
left a comment
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.
Great work!! I haven't tested the code myself but I went through the changes and they look straightforward. The docs are clear and @kanekosh 's updated tests look good - nice job with the comments.
I wonder if this PR warrants a minor release?
Purpose
Closes #374, and hopefully addresses some of the issues we've been having. I also added the long-awaited numpy2 test on CI (with windows).
Some to dos:
Expected time until merged
Type of change
Testing
Checklist
flake8andblackto make sure the Python code adheres to PEP-8 and is consistently formattedfprettifyor C/C++ code withclang-formatas applicable