Refactor moi_function and jump_function into separate file#4190
Conversation
| 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).
There was a problem hiding this comment.
It's the new way forward but we don't have to enforce it. It's for clarifying in the comments& doc
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4190 +/- ##
==========================================
- Coverage 99.93% 99.85% -0.08%
==========================================
Files 42 43 +1
Lines 6305 6307 +2
==========================================
- Hits 6301 6298 -3
- Misses 4 9 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Okay @blegat how is this. |
blegat
left a comment
There was a problem hiding this comment.
Looks good modulo my minor comments to improve readability
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.