-
-
Notifications
You must be signed in to change notification settings - Fork 394
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
Add GenericModel{T} and GenericVariableRef{T} #3377
Conversation
f81fe3f
to
0ea2871
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #3377 +/- ##
==========================================
- Coverage 97.97% 97.97% -0.01%
==========================================
Files 34 34
Lines 4930 4979 +49
==========================================
+ Hits 4830 4878 +48
- Misses 100 101 +1
☔ View full report in Codecov by Sentry. |
The last commit, 825459b, fixes a number of cases that still used Since this PR is extracted from #3191, which is passing tests, I assume that this means we don't have good test coverage for non-Float64 number types. |
Rebasing this now since it was my fault |
825459b
to
f23f543
Compare
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.
I think we should still hold off merging this for now. But I'm a lot more confident in these changes than when it was in the monolithic PR.
19c203f
to
28a2bb1
Compare
src/JuMP.jl
Outdated
""" | ||
value_type(::Type{<:AbstractModel}) = Float64 | ||
|
||
mutable struct GenericModel{T} <: AbstractModel |
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.
Can we use a more descriptive name instead of T
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.
Maybe it's enough if it's in the docstring ? Having a small name might make the user thing there is a simple interpretation and no need to look up the docstring. At least with T
it forces the user to read the docstring and get the full explanation and not get a wrong understanding ^^
d60045e
to
fbfd2ad
Compare
@blegat take a look at the last commit I pushed. We can always revert, but any reason why we didn't add a |
I guess it's a bit weird to have |
Yeah this is weird. But people would really do julia> GenericModel{Float64}(value_type = Int)
A JuMP Model
Feasibility problem with:
Variables: 0
Model mode: AUTOMATIC
CachingOptimizer state: NO_OPTIMIZER
Solver name: No optimizer attached.
julia> Model(value_type = Int)
A JuMP Model
Feasibility problem with:
Variables: 0
Model mode: AUTOMATIC
CachingOptimizer state: NO_OPTIMIZER
Solver name: No optimizer attached.
We cannot do this. It would export |
Then what about |
The documented API detail is It's an implementation detail that |
34a9b14
to
036d014
Compare
We need a keyword argument for I guess we should just check whether having the constructor |
src/JuMP.jl
Outdated
function direct_model(backend::MOI.ModelLike) | ||
function direct_model( | ||
backend::MOI.ModelLike; | ||
value_type::Type{T} = Float64, |
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.
@blegat I already added the kwarg for direct_model
I don't think there's any technical issue with it. Stylistically it's weird only if you know that |
@mlubin Any opinion on this ? |
4a55ebc
to
585630b
Compare
372f638
to
ebe5b47
Compare
Could we make |
|
ebe5b47
to
70a1ccb
Compare
af1eda9
to
e9bdd50
Compare
This is a non-breaking feature addition that prepares the way for generic number types in JuMP.
This is another step in the process of adding generic number support to JuMP. These commits are separate to make them easier to review and to minimize the risk of introducing breaking changes.
7718087
to
43178a1
Compare
Looks good to merge |
Let's wait for @mlubin to take another look. |
Co-authored-by: Miles Lubin <mlubin@google.com>
Following #3191 (comment), this PR takes out from #3191 the part of src that should be boilerplate and not prone to generate argument to allow the important parts to stand out from #3191 and also to be able to merge this big part that is prone to create conflicts.