-
-
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 support for generic number type #3385
Conversation
f7300eb
to
950dd94
Compare
b473e8a
to
8e95c7a
Compare
@blegat this is a lot easier to review and see what is going on. |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #3385 +/- ##
==========================================
- Coverage 97.97% 97.91% -0.06%
==========================================
Files 34 34
Lines 4979 4993 +14
==========================================
+ Hits 4878 4889 +11
- Misses 101 104 +3
☔ View full report in Codecov by Sentry. |
@@ -587,7 +587,7 @@ function test_extension_SDP_errors( | |||
"In `@constraint(model, [x 1; 1 -y] >= [1 x; x -2], PSDCone(), unknown_kw = 1)`:" * | |||
" Unrecognized constraint building format. Tried to invoke " * | |||
"`build_constraint(error, $(aff_str)[x - " * | |||
"1 -x + 1; -x + 1 -y + 2], $(MOI.GreaterThan(0.0)), $(PSDCone()); unknown_kw = 1)`, but no " * | |||
"1 -x + 1; -x + 1 -y + 2], $(MOI.GreaterThan(false)), $(PSDCone()); unknown_kw = 1)`, but no " * |
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.
Breaking feature 2: the fallback for a >= b
is now a - b in GreaterThan(false)
instead of a - b in GreaterThan(0.0)
. But I don't know if anyone is actually intercepting this method. And changes to the method dispatch are already potentially breaking. Extensions should be intercepting things higher up the call stack.
cc @goulart-paul, here's a demo with Clarabel's https://jump.dev/JuMP.jl/previews/PR3385/tutorials/conic/arbitrary_precision/ |
@odow : thanks for doing that. Should work also for BigFloat SDPs as of v0.5 too. I suspect we could get it to work also with Would that be of interest? |
Yes. That was the next thing I tried, and I got stopped at (I'm not sure it's that simple though? The rational simplex walks the vertices, so everything is always rational. An interior point type thing in rational arithmetic seems much harder? Maybe I'm missing something obvious.) |
Good point - entirely possible that the solver would misbehave. I was thinking only at the "will it crash" sort of level. We will give it at a try anyway. Edit: I could get it to work in the sense that it would do an iteration and not crash, but the results are ugly and it's extremely slow. I think it is not really useable with Rationals for the reason posited by @odow above. |
Another data point: this works with MiniZinc and julia> using JuMP
julia> import MiniZinc
julia> model = GenericModel{Int}(() -> MiniZinc.Optimizer{Int}(MiniZinc.Chuffed()))
A JuMP Model
Feasibility problem with:
Variables: 0
Model mode: AUTOMATIC
CachingOptimizer state: EMPTY_OPTIMIZER
Solver name: MiniZinc
julia> @variable(model, 1 <= x[1:3] <= 3, Int)
3-element Vector{GenericVariableRef{Int64}}:
x[1]
x[2]
x[3]
julia> @variable(model, z[1:2], Bin)
2-element Vector{GenericVariableRef{Int64}}:
z[1]
z[2]
julia> @constraint(model, z[1] <--> {[x[1], x[2]] in MOI.AllDifferent(2)})
z[1] <--> {[x[1], x[2]] ∈ MathOptInterface.AllDifferent(2)}
julia> @constraint(model, z[2] <--> {[x[2], x[3]] in MOI.AllDifferent(2)})
z[2] <--> {[x[2], x[3]] ∈ MathOptInterface.AllDifferent(2)}
julia> @constraint(model, z[1] + z[2] == 1)
z[1] + z[2] = 1
julia> optimize!(model)
Warning: included file "count.mzn" overrides a global constraint file from the standard library. This is deprecated. For a solver-specific redefinition of a global constraint, override "fzn_<global>.mzn" instead.
Warning: included file "global_cardinality_low_up.mzn" overrides a global constraint file from the standard library. This is deprecated. For a solver-specific redefinition of a global constraint, override "fzn_<global>.mzn" instead.
julia> value.(x)
3-element Vector{Int64}:
2
3
3
julia> value.(z)
2-element Vector{Int64}:
1
0 |
950dd94
to
a055d32
Compare
4d59a40
to
34a9b14
Compare
34a9b14
to
036d014
Compare
372f638
to
ebe5b47
Compare
8672cee
to
e226881
Compare
1661808
to
af1eda9
Compare
e226881
to
7599cbe
Compare
af1eda9
to
e9bdd50
Compare
7599cbe
to
38e2d24
Compare
7718087
to
43178a1
Compare
38e2d24
to
5058481
Compare
Okay, I've reverted to the |
80957a6
to
e504b55
Compare
This is ready for review now that it is rebased on |
@mlubin this would be nice to get in before JuMP-dev? |
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.
That was easy to review. Good idea splitting into smaller parts.
end | ||
|
||
function model_convert(model::AbstractModel, α::Number) | ||
T = value_type(typeof(model)) |
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.
Is querying value_type
in these functions as efficient as explicitly grabbing T from the from the AbstractModel{T}
? This code is in the hot path from what I can tell.
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.
Note that this is AbstractModel
, not GenericModel{T}
.
This is a little slower (extra function calls, etc), but not by much.
julia> using JuMP
julia> model = Model();
julia> f(m) = value_type(typeof(model))
f (generic function with 1 method)
julia> g(m::GenericModel{T}) where {T} = T
g (generic function with 1 method)
julia> @code_native f(model)
.section __TEXT,__text,regular,pure_instructions
; ┌ @ REPL[3]:1 within `f'
subq $24, %rsp
movq %rsi, 16(%rsp)
movabsq $5387182984, %rax ## imm = 0x14119E388
movq (%rax), %rax
movq -8(%rax), %rax
andq $-16, %rax
movq %rax, 8(%rsp)
movabsq $jl_apply_generic, %rax
movabsq $5686588464, %rdi ## imm = 0x152F27430
leaq 8(%rsp), %rsi
movl $1, %edx
callq *%rax
addq $24, %rsp
retq
; └
julia> @code_native g(model)
.section __TEXT,__text,regular,pure_instructions
; ┌ @ REPL[9]:1 within `g'
movq %rsi, -8(%rsp)
movabsq $jl_system_image_data, %rax
retq
; └
julia> @time f(model)
0.000007 seconds (2 allocations: 128 bytes)
Float64
julia> @time f(model)
0.000006 seconds (2 allocations: 128 bytes)
Float64
julia> @time g(model)
0.000003 seconds
Float64
julia> @time g(model)
0.000003 seconds
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.
Oh. I can't type. They're exactly the same:
julia> using JuMP
julia> model = Model();
julia> f(m) = value_type(typeof(m))
f (generic function with 1 method)
julia> g(m::GenericModel{T}) where {T} = T
g (generic function with 1 method)
julia> @code_native f(model)
.section __TEXT,__text,regular,pure_instructions
; ┌ @ REPL[10]:1 within `f'
movq %rsi, -8(%rsp)
movabsq $jl_system_image_data, %rax
retq
; └
julia> @code_native g(model)
.section __TEXT,__text,regular,pure_instructions
; ┌ @ REPL[11]:1 within `g'
movq %rsi, -8(%rsp)
movabsq $jl_system_image_data, %rax
retq
; └
Co-authored-by: Benoît Legat <benoit.legat@gmail.com>
e504b55
to
d5f9a7d
Compare
@blegat it's been a couple of weeks, so still good to go? |
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.
Yes I'm good with the changes since my last review
I figured I'd leave #3191 alone so we have a baseline in case anything got mucked up during the split of the PRs and all the various rebasing.
Tutorial: https://jump.dev/JuMP.jl/previews/PR3385/tutorials/conic/arbitrary_precision/
Closes #2025