-
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
Added linear constraint check and updated documentation #410
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #410 +/- ##
===========================================
- Coverage 74.49% 63.39% -11.10%
===========================================
Files 22 22
Lines 3317 3325 +8
===========================================
- Hits 2471 2108 -363
- Misses 846 1217 +371 ☔ View full report in Codecov by Sentry. |
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.
No additional comments on the check we came up with, thanks for fixing the tests!!
return funcs, fail | ||
|
||
|
||
def objfunc_no_con(xdict): |
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.
Nice catch, I assume the new check had these tests failing right away, right?
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.
Yes you're right
Do we have a unit test that confirms the standard way of adding linear constraints is working as expected? If not, can that be added to this PR? If it exists and I missed it, then point me to the test and I'll approve this PR. This would just be as simple as adding a few linear constraints to a valid opt prob and then making sure the number of linear constraints and the Jacobian are correct. |
I don't know if there's a test that checks the number of linear constraints, but we already have a few tests that include linear constraints and assert equal on the optimal solution, which validates that linear constraints are properly imposed. |
That's good enough for me. Thanks for the double check! |
Purpose
pyOptSparse computes the linear constraints exclusively based on
jac
,lower
, andupper
entries ofaddConGroup
, and it ignores the linear constraint value returned by the user-defined obj/con function. This is confusing and can result in unexpected behavior (see related #296).This PR adds a linear constraint checkerto make sure that a user-defined obj/con function does not return linear constraint values. If it does, pyOptSparse will raise an error. I also updated the documentation accordingly.
I fixed a few existing tests that have been computing linear constraint values in the obj/con function (which we should not). I also added a test to check if pyOptSparse raises an error as expected.
This change will break some existing runscripts that return linear constraint values in the user-defined function. OpenMDAO's pyOptSparseDriver will be fine. Not sure about MACH runscripts (haven't checked).
Expected time until merged
Type of change
Testing
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