-
-
Notifications
You must be signed in to change notification settings - Fork 402
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
Fix method ambiguities in JuMP.Containers #3173
Conversation
Codecov ReportBase: 97.72% // Head: 98.07% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #3173 +/- ##
==========================================
+ Coverage 97.72% 98.07% +0.34%
==========================================
Files 33 33
Lines 4522 4563 +41
==========================================
+ Hits 4419 4475 +56
+ Misses 103 88 -15
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
function Base.similar( | ||
A::DenseAxisArray{T,N,Ax}, | ||
::Type{S}, | ||
axes::Ax, | ||
) where {T,N,Ax,S} | ||
) where {T,N,Ax<:Tuple{<:AbstractVector},S} |
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 also support axes that are not AbstractVector
at the moment so that would be breaking
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 type allows it, but we convert everything to AbstractVector
on creation: #2713.
JuMP.jl/src/Containers/DenseAxisArray.jl
Lines 158 to 206 in 9f61e95
_abstract_vector(x::AbstractVector) = x | |
function _abstract_vector(x::AbstractVector{<:CartesianIndex}) | |
return error( | |
"Unsupported index type `CartesianIndex` in axis: $x. Cartesian " * | |
"indices are restricted for indexing into and iterating over " * | |
"multidimensional arrays.", | |
) | |
end | |
_abstract_vector(x) = _abstract_vector([a for a in x]) | |
_abstract_vector(x::AbstractArray) = vec(x) | |
function _abstract_vector(x::Number) | |
@warn( | |
"Axis contains one element: $x. If intended, you can safely " * | |
"ignore this warning. To explicitly pass the axis with one " * | |
"element, pass `[$x]` instead of `$x`.", | |
) | |
return _abstract_vector([x]) | |
end | |
""" | |
DenseAxisArray(data::Array{T, N}, axes...) where {T, N} | |
Construct a JuMP array with the underlying data specified by the `data` array | |
and the given axes. Exactly `N` axes must be provided, and their lengths must | |
match `size(data)` in the corresponding dimensions. | |
# Example | |
```jldoctest; setup=:(using JuMP) | |
julia> array = JuMP.Containers.DenseAxisArray([1 2; 3 4], [:a, :b], 2:3) | |
2-dimensional DenseAxisArray{Int64,2,...} with index sets: | |
Dimension 1, Symbol[:a, :b] | |
Dimension 2, 2:3 | |
And data, a 2×2 Array{Int64,2}: | |
1 2 | |
3 4 | |
julia> array[:b, 3] | |
4 | |
``` | |
""" | |
function DenseAxisArray(data::Array{T,N}, axs...) where {T,N} | |
@assert length(axs) == N | |
new_axes = _abstract_vector.(axs) # Force all axes to be AbstractVector! | |
return DenseAxisArray(data, new_axes, build_lookup.(new_axes)) | |
end |
Urgh. #3152 just added new ambiguities. I think that shows we need to get these tests in ASAP to keep a lid on things. |
No description provided.