Add GenericModel{T} and GenericVariableRef{T}

[blegat]

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.

[codecov]

Codecov Report

Patch coverage: 99.44% and project coverage change: -0.01 ⚠️

Comparison is base (0d1d90a) 97.97% compared to head (bc9a2b3) 97.97%.

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     

Impacted Files	Coverage Δ
src/copy.jl	96.19% <93.33%> (+0.03%)	⬆️
src/variables.jl	98.76% <99.23%> (-0.17%)	⬇️
src/JuMP.jl	94.87% <100.00%> (+0.20%)	⬆️
src/aff_expr.jl	97.28% <100.00%> (ø)
src/callbacks.jl	100.00% <100.00%> (ø)
src/constraints.jl	96.56% <100.00%> (ø)
src/feasibility_checker.jl	100.00% <100.00%> (ø)
src/file_formats.jl	100.00% <100.00%> (ø)
src/lp_sensitivity2.jl	97.79% <100.00%> (ø)
src/macros.jl	98.63% <100.00%> (ø)
... and 8 more

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[odow]

The last commit, 825459b, fixes a number of cases that still used Model, VariableRef, AffExpr, or QuadExpr. I've gone through and audited every instance of these types remaining in /src, and I think I have them all now (except for src/nlp.jl).

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.

[odow]

Rebasing this now since it was my fault

[odow]

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.

[odow]

Can we use a more descriptive name instead of T

[blegat]

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 ^^

[odow]

@blegat take a look at the last commit I pushed. We can always revert, but any reason why we didn't add a value_type keyword argument to Model in the first place?

[blegat]

I guess it's a bit weird to have GenericModel{Float64}(value_type = BigFloat).
Maybe model(value_type = BigFloat) and direct_model(value_type = BigFloat) ?

[odow]

GenericModel{Float64}(value_type = BigFloat)

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.

model(value_type = BigFloat)

We cannot do this. It would export model and conflict with everyone using model as the name of their Model.

[blegat]

model(value_type = BigFloat)

We cannot do this. It would export model and conflict with everyone using model as the name of their Model.

Then what about generic_model(value_type = BigFloat) and direct_model(value_type = BigFloat) ?

[odow]

The documented API detail is Model(; value_type). It's the most JuMP-like API, and I don't think that's confusing, or that there is any ambiguity.

It's an implementation detail that Model is also a synonym for GenericModel{Float64}. We'd never actually document people using the GenericModel{T} constructor.

[blegat]

We need a keyword argument for direct_model as well. At the moment Model is hardcoded in direct_model. I always found direct_model vs Model a bit inconsistent so I thought it was a good occasion to have direct_model vs generic_model but the generic vs direct is maybe not the best way to describe the difference between direct_model and Model.

I guess we should just check whether having the constructor Model not create a Model violates one of the Julia rules for constructors

[odow]

@blegat I already added the kwarg for direct_model

[odow]

I guess we should just check whether having the constructor Model not create a Model violates one of the Julia rules for constructors

I don't think there's any technical issue with it. Stylistically it's weird only if you know that Model is an alias. We just never mention the GenericModel constructor.

[blegat]

@mlubin Any opinion on this ?

[mlubin]

Model(; value_type) and direct_model(; value_type) seems reasonable to me. It's the least disruptive. Exporting model is a no-go because of so many local variables named model. Will Julia specialize on keyword argument types in this context (i.e., will the return value of Model(; value_type=T) infer as GenericModel{T})?

Could we make GenericModel{Float64}(value_type = BigFloat) an explicit error?

[odow]

• Check type inference of Model() and Model(; value_type = BigFloat)
• If inference fails, switch back to GenericModel{T}
• direct_generic_model(::Type{T}, optimizer) where {T<:Real}
• direct_model{T}(optimizer)
• model = @model()

[odow]

So I guess the inference failure is a kicker:

Let me revert to what we had before.

[blegat]

Looks good to merge

[odow]

Looks good to merge

Let's wait for @mlubin to take another look.