-
-
Notifications
You must be signed in to change notification settings - Fork 395
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
Improve inferrability of JuMP functions #1846
Conversation
There's a subtle difference in regular type assertions and return value type assertions: julia> f() = 1
f (generic function with 1 method)
julia> function g()
return f()::Float64
end
g (generic function with 1 method)
julia> function h()::Float64
return f()
end
h (generic function with 1 method)
julia> g()
ERROR: TypeError: in g, in typeassert, expected Float64, got Int64
Stacktrace:
[1] g() at ./REPL[2]:2
[2] top-level scope at none:0
julia> h()
1.0 Which should we prefer? Also we should distinguish the cases where MOI defines or should define an expected return type like |
Use a type assertion when MOI defines what type is returned. function objective_sense(model::Model)
return MOI.get(model, MOI.ObjectiveSense())::MOI.OptimizationSense
end Use a return type assertion for JuMP-specific values. function objective_value(model::Model)::Float64
return MOI.get(model, MOI.ObjectiveValue())
end |
I agree with @odow suggestion. Some solvers might return a |
I have move type asserts to return value type assertion when appropriate |
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.
Larger architectural question: which functions are we guaranteeing are type stable and which are we not, and how do we document this?
src/JuMP.jl
Outdated
@@ -384,7 +384,7 @@ end | |||
|
|||
Returns number of variables in `model`. | |||
""" | |||
num_variables(model::Model) = MOI.get(model, MOI.NumberOfVariables()) | |||
num_variables(model::Model) = MOI.get(model, MOI.NumberOfVariables())::Int64 |
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.
What if a solver returns an Int32?
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.
We should use a conversion here, that is also consistent with num_constraint
in #1850
I think that by default all jump function should be type stable (or with a Union with |
ef92b2f
to
1ce30ae
Compare
As the MOI backend does not have a concrete type, Julia is not able to infer the result of the
MOI.get
calls. This PR adds type asserts to help inferrability and adds tests to unsure that the functions are now inferrable.Closes #1824