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

Shall we move part of Ipopt's MOI wrapper to a new MOI Utilities? #1169

Closed
frapac opened this issue Sep 28, 2020 · 5 comments
Closed

Shall we move part of Ipopt's MOI wrapper to a new MOI Utilities? #1169

frapac opened this issue Sep 28, 2020 · 5 comments

Comments

@frapac
Copy link
Contributor

frapac commented Sep 28, 2020

Reading this new PR: jump-dev/NLopt.jl#150
it looks like several packages are re-implementing Ipopt's MOI wrapper. A non-exhaustive list being

I am wondering if at some point, it would make sense to implement the common parts of these different wrappers as a MOI Utilities (e.g. the part to plug the linear and quadratic expressions in the MOI.AbstractNLPEvaluator)? I think in the long term that would ease the maintenance of the aforementioned packages.

@mlubin
Copy link
Member

mlubin commented Sep 28, 2020

I agree the code duplication isn't great, but we also have to be careful that any solution doesn't actually make the solver wrappers harder to read. Writing and reading MOI wrappers is already complex enough. We learned from LinQuadOptInterface that intermediate abstractions can make the wrappers harder to understand because they don't fully alleviate the need for working at the MOI level, so the wrapper has to deal with MOI and the intermediate level at the same time.

I think in the long term that would ease the maintenance of the aforementioned packages.

What maintenance issues have been tied to borrowing the structure of Ipopt's MOI wrapper? I don't think any changes to the structure have been needed in the past two years.

@frapac
Copy link
Contributor Author

frapac commented Sep 28, 2020

I understand. The argument against intermediate abstractions is a good one.
I don't have any maintenance issue in mind (I think the last issue on Ipopt's wrapper was the sign of the Hessian set to the opposite when we were maximizing, but it was fixed long time ago). The main point was to have more concise MOI wrapper for the different NLP optimizers. But the return of experience from LinQuadOptInterface is compelling, indeed.

@odow
Copy link
Member

odow commented Sep 28, 2020

We learned from LinQuadOptInterface that intermediate abstractions can make the wrappers harder to understand because they don't fully alleviate the need for working at the MOI level, so the wrapper has to deal with MOI and the intermediate level at the same time

💯 I'm vetoing intermediate abstractions.

@blegat
Copy link
Member

blegat commented Sep 29, 2020

The large amount of copy-paste should be resolved by #846.
For instance, a lot of duplicate work is to rewrite linear and quadratic constraints as nonlinear ones: After the NLP rewrite, this will be done by bridges so we can just get rid of all this code.

@frapac
Copy link
Contributor Author

frapac commented Sep 29, 2020

Closing this issue in favor of #846

@frapac frapac closed this as completed Sep 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants