jump-dev / ParametricOptInterface.jl

Extension for dealing with parameters
MIT License
37 stars 7 forks source link

[breaking] drop invalid nonlinear support #147

Closed odow closed 4 months ago

odow commented 8 months ago

Closes https://github.com/jump-dev/ParametricOptInterface.jl/issues/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.

codecov[bot] commented 8 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

:exclamation: 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 ```diff @@ 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 ```

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

joaquimg commented 8 months ago

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 commented 8 months ago
  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 commented 4 months ago

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

joaquimg commented 4 months ago

Lets merge this