-
Notifications
You must be signed in to change notification settings - Fork 58
Provide the user_data to the callbacks #514
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
base: master
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #514 +/- ##
===========================================
- Coverage 100.00% 99.48% -0.52%
===========================================
Files 5 5
Lines 1154 1170 +16
===========================================
+ Hits 1154 1164 +10
- Misses 0 6 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I'm not really in favour of this. What's the problem with a closure? |
|
Do you have a worked example of code that would be improved with this? |
|
Can't this just be achieved with: prob = Ipopt.CreateIpoptProblem(
n,
x_L,
x_U,
m,
g_L,
g_U,
nnzJ,
nnzH,
x -> eval_f(user_data, x),
(args...) -> eval_g(user_data, args...),
(args...) -> eval_grad_f(user_data, args...),
(args...) -> eval_jac_g(user_data, args...),
(args...) -> eval_h(user_data, args...),
)I don't really want to make changes to the C API, and adding multiple ways of doing something has a long-term maintenance cost. |
|
You can convince me of the need for this at JuMP-dev |
|
Conclusion is that we need an example of why we need this. It might have something to do with Enzyme. |
| )::Cint | ||
| try | ||
| return reenable_sigint() do | ||
| prob = unsafe_pointer_to_objref(user_data)::IpoptProblem |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we need IpoptProblem{M} here to avoid type-instabilities ?
@odow
When I created the C / Julia interface for
Uno, I checked a few times the interfaces of other nonlinear solvers (Ipopt,KNITRO,GALAHAD) and I noticed that inIpopt.jlwe never provide a way to passuser_data.This means that
model::Optimizeris passed as a closure, and the same applies tomodel::AbstractNLPModelinNLPModelsIpopt.jl.In
UnoSolver.jl, I added an option to passuser_data/user_modelwhen creating the internal solver model (Ipopt.IpoptProblemin this repository).If
user_dataisnothing, we keep the current signature; otherwise, we passuser_data/user_modelas the first argument to all Julia callbacks.This is what we expect in the signatures of the NLP API in
MathOptInterface.jlorNLPModels.jl.I also think it is a clean way to pass a list of parameters to an objective or constraints.
For example, if we have a complex physical problem, we would like to store them in a Julia structure and pass this structure as an argument to the Julia functions.
I find it neater than using closures.
The current PR is non-breaking because, by default,
user_data/user_modelisnothing.I would like to have your point of view (as well as that of other users) before I add unit tests for coverage.
Ipopt.jl: https://github.com/jump-dev/Ipopt.jl/blob/master/ext/IpoptMathOptInterfaceExt/MOI_wrapper.jl#L1242-L1264UnoSolver.jl: https://github.com/cvanaret/Uno/blob/main/interfaces/Julia/ext/UnoSolverMathOptInterfaceExt/MOI_wrapper.jl#L1294-L1301