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

[breaking] drop invalid nonlinear support #147

Merged
merged 2 commits into from
Jun 25, 2024
Merged

[breaking] drop invalid nonlinear support #147

merged 2 commits into from
Jun 25, 2024

Conversation

odow
Copy link
Member

@odow odow commented Mar 1, 2024

Closes #146
Closes #149

We actually don't support the NLPBlock because POI re-orders the variables by removing parameters. The tests passed because they had parameters at the end. But that isn't a good solution, and it seems simpler to drop support for the legacy nonlinear interface than throw an error if parameters are not in the right order.

We also don't really need POI for nonlinear. Ipopt supports parameters directly.

Copy link

codecov bot commented Mar 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.92%. Comparing base (4ec565a) to head (ec7ed41).

Current head ec7ed41 differs from pull request most recent head 343f7b3

Please upload reports for the commit 343f7b3 to get more accurate results.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #147      +/-   ##
==========================================
- Coverage   94.96%   94.92%   -0.04%     
==========================================
  Files           5        5              
  Lines        1032     1025       -7     
==========================================
- Hits          980      973       -7     
  Misses         52       52              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@joaquimg
Copy link
Member

joaquimg commented Mar 1, 2024

1 - Is there a tutorial using NLP parameters?
2 - In theory, we could preprocess the new nonlinear expression for VariableRef-in-MOI.Parameter, but it might be messy, correct?

@odow
Copy link
Member Author

odow commented Mar 1, 2024

  1. If you're using Ipopt, you don't need POI. Syntax is the same.
  2. Yes, we could support MOI.ScalarNonlinearFunction. The issue is that we can't rewrite NLPBlock.

@odow
Copy link
Member Author

odow commented Jun 24, 2024

Bump. I get that this is breaking, but the previous approach was incorrect and can't be worked around.

@joaquimg
Copy link
Member

Lets merge this

@odow odow merged commit cde9813 into master Jun 25, 2024
6 checks passed
@odow odow deleted the od/drop-nlp-support branch June 25, 2024 00:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

default_copy_to accesses private methods Invalid variable ordering with old nonlinear interface
2 participants