pyoptsparse icon indicating copy to clipboard operation
pyoptsparse copied to clipboard

Added linear constraint check and updated documentation

Open kanekosh opened this issue 1 year ago • 3 comments

Purpose

pyOptSparse computes the linear constraints exclusively based on jac, lower, and upper entries of addConGroup, 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

  • [x] Bugfix (non-breaking change which fixes an issue)
  • [ ] New feature (non-breaking change which adds functionality)
  • [ ] Breaking change (non-backwards-compatible fix or feature)
  • [ ] Code style update (formatting, renaming)
  • [ ] Refactoring (no functional changes, no API changes)
  • [x] Documentation update
  • [ ] Maintenance update
  • [ ] Other (please describe)

Testing

Checklist

  • [ ] I have run flake8 and black to make sure the Python code adheres to PEP-8 and is consistently formatted
  • [ ] I have formatted the Fortran code with fprettify or C/C++ code with clang-format as applicable
  • [x] I have run unit and regression tests which pass locally with my changes
  • [x] I have added new tests that prove my fix is effective or that my feature works
  • [x] I have added necessary documentation

kanekosh avatar Jun 26 '24 20:06 kanekosh

Codecov Report

Attention: Patch coverage is 75.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 63.39%. Comparing base (7376d71) to head (89e4935). Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
pyoptsparse/pyOpt_optimizer.py 75.00% 2 Missing :warning:
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     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Jun 26 '24 20:06 codecov[bot]

@lamkina @ewu63 @sseraj can you check and merge this soon so we can go ahead and make a new release?

marcomangano avatar Jul 03 '24 10:07 marcomangano

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.

lamkina avatar Jul 04 '24 22:07 lamkina

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.

kanekosh avatar Jul 06 '24 03:07 kanekosh

That's good enough for me. Thanks for the double check!

lamkina avatar Jul 06 '24 03:07 lamkina