Refactor moi_function and jump_function into separate file#4190
Conversation
| function MOI.submit(model::GenericModel, cb::MOI.UserCut, con::ScalarConstraint) | ||
| return MOI.submit(backend(model), cb, moi_function(con.func), con.set) | ||
| f = moi_function(model, con.func) | ||
| return MOI.submit(backend(model), cb, f, con.set) |
There was a problem hiding this comment.
How did we miss them ? Removing the moi_function method was precisely done to catch them
There was a problem hiding this comment.
We don't have any nonlinear callbacks.
| check_belongs_to_model(f, model) | ||
| return moi_function(f) | ||
| end | ||
|
|
There was a problem hiding this comment.
So this is a very weird method. It's not just a fallback. I think the only reason it was needed was because of check_belongs_to_model?
There was a problem hiding this comment.
Yes, since we moved to requiring the user to call moi_function, I thought we might as well do check_belongs_to_model there since these two are always called one after the other
| name::String = "", | ||
| ) | ||
| con = model_convert(model, con) | ||
| check_belongs_to_model(con, model) |
There was a problem hiding this comment.
We just do it explicitly here now instead.
There was a problem hiding this comment.
If we're going to call check_belongs_to_model every time we call moi_function (which makes sense), we might as well just call it inside moi_function no ? Since moi_function takes 2 arguments, it makes sense to check that they model is indeed the correct one, doesn't it ?
|
|
||
| # A default fallback for backwards compatibility. The first argument `model` was | ||
| # introduced in JuMP@1.31.0. | ||
| moi_function(model, f) = moi_function(f) |
There was a problem hiding this comment.
And now I have a much more sensible fallback.
There was a problem hiding this comment.
I think the definition should be: The user should always call moi_function(model, f). When you implement, you can either implement moi_function(model, f) (and checking that the model match) or just implement moi_function(f).
To me, it makes sense to check that the model match precisely when we transform the JuMP function to MOI since that's when the variable references are going to be turned into indices (so without the model). Implementing it in two steps 1) check membership and then 2) do a moi_function(f) is one way to do it but they could also just be done together, which is what we do for NonlinearExpr
There was a problem hiding this comment.
We can't do that because that's then a breaking change. People can and have been calling moi_function(f).
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4190 +/- ##
==========================================
- Coverage 99.93% 99.69% -0.24%
==========================================
Files 42 43 +1
Lines 6305 6307 +2
==========================================
- Hits 6301 6288 -13
- Misses 4 19 +15 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
odow
changed the title
|
|
||
| # A backwards-compatible function to preserve behavior prior to #4032. This was | ||
| # used by Plasmo.jl | ||
| function moi_function(f::GenericNonlinearExpr{V}) where {V} |
There was a problem hiding this comment.
I still think that this should just grab model from the expression graph and then redirect to moi_function(model, f). Having such a subtle behavior (aliases kept or not), depend on whether you call two very similar methods.
There was a problem hiding this comment.
There are trivial examples where the model doesn't exist. NonlinearExpr(:+, Any[0.0])
|
Okay @blegat how is this. |
2 participants
x-ref #4189
We don't necessarily need to merge this. But these functions were spread all over the place. I'll comment in-line on something.