-
Notifications
You must be signed in to change notification settings - Fork 97
Infinite default bounds for AbstractFloat #1331
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
Conversation
| _no_lower_bound(::Type{T}) where {T} = zero(T) | ||
| _no_lower_bound(::Type{T}) where {T<:AbstractFloat} = typemin(T) | ||
| _no_upper_bound(::Type{T}) where {T} = zero(T) | ||
| _no_upper_bound(::Type{T}) where {T<:AbstractFloat} = typemax(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.
Why is zero(T) necessary and not just typemin(T) and typemax(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.
Answered my own question:
julia> typemax(Complex{Int})
ERROR: MethodError: no method matching typemax(::Type{Complex{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.
Also, typemax(Int) is a bit scary with overflows. Maybe we can say that we don't guarantee anything for non-AbstractFloat so that we can change our mind in the future
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.
The other option is to make the bound vectors Union{Nothing,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.
Who uses AbstractModel with something other than an AbstractFloat at present?
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.
The advantage here is that Clp can directly take the Vector{Float64} and pass it to the inner solver without the need to do any transformation: https://github.com/jump-dev/Clp.jl/pull/111/files#diff-18c6af231f3facc5ba10dd45231da349efda22ecd8d68a4661dee8027bc413a7R279-R280.
We already have a vector of flags that contains the information on whether a bound is set or not.
So the value of the bound if it is not set does not alter the MOI API.
Using Union{Nothing,T} would make things slower, unallow Clp and friends to use it directly and would not bring any advantage.
odow
left a comment
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.
For now, this is at least just a copy of existing behavior for non-floats.
Needed for jump-dev/Clp.jl#111. With this PR,
lower_boundandupper_boundare now part of the API since it's documented so Clp can use it directly.