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 dense and sparse wrappers for ParOpt from ParOpt directly #414

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

gjkennedy
Copy link
Contributor

@gjkennedy gjkennedy commented Oct 2, 2024

Purpose

This PR adds the ParOpt sparse and dense constraint wrappers for pyOptSparse from ParOpt directly.

Expected time until merged

Type of change

  • 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)
  • 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
  • I have run unit and regression tests which pass locally with my changes
  • I have added new tests that prove my fix is effective or that my feature works
  • I have added necessary documentation

@gjkennedy gjkennedy requested a review from a team as a code owner October 2, 2024 14:44
@A-CGray A-CGray requested review from ewu63 and removed request for lamkina October 2, 2024 14:55
@A-CGray
Copy link
Member

A-CGray commented Oct 2, 2024

@marcomangano @ewu63 the actual pyoptsparse wrapper that this will end up using is here

@A-CGray
Copy link
Member

A-CGray commented Oct 2, 2024

The tests are failing because our docker images are pinned to an older version of ParOpt. But if we update the ParOpt version in the docker images before merging this PR then the image builds will fail and so the tests here will still fail. So I guess we need to test this PR locally, merge it even with failing azure tests, and then update the docker images. Opinions @ewu63 @eirikurj ?

@A-CGray
Copy link
Member

A-CGray commented Oct 2, 2024

@gjkennedy I've run the pyoptsparse tests locally, the dense version of your wrapper seems to work fine, but when using the sparse version the constraints seem to be ignored if you use the trust region algorithm which is the default, interior-point and mma work fine. Try running test_hs015.py

@marcomangano
Copy link
Contributor

Specific test errors aside, I have mixed feelings about moving the actual pyOptSparse wrapper to the parOpt repo.

On the one hand, outsourcing it could make things a bit easier for us in terms of maintenance, especially if the SMDO group plans to update the python API on a regular basis.
On the other hand, if we make breaking changes to our codebase, we would not be able to address it directly and have to wait for a new version release of ParOpt for our tests to pass. Since we run tests on our side, I am leaning towards moving the updated wrapper back here.

This would also avoid the kind of "circular import" happening now, where pyoptsparse imports paropt but the imported wrapper is also importing pyoptsparse components - but this is a minor note.
Curious to hear @ewu63 and @eirikurj take on this.

@ewu63
Copy link
Collaborator

ewu63 commented Oct 3, 2024

  1. I have no idea how this circular dependency would be resolved, and in any case I don't think pyoptsparse-specific code should live in ParOpt. Instead, a generic Python interface should be provided. I think the correct approach would be the following, consistent with our other wrappers
    1. Keep the wrapper in this repo
    2. Test with specific tagged version, and request that changes to API be versioned appropriately
  2. Once the wrapper is confirmed to be working, we can merge this PR and then update docker. The procedure you described is fine @A-CGray

@gjkennedy
Copy link
Contributor Author

Hey guys, this is a PR. Feel free to accept or reject it. Do not feel free to copy code from the wrapper I wrote into your repo! Thanks!

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.

4 participants