-
Notifications
You must be signed in to change notification settings - Fork 14
fixes for breakage introduced by recent Julia versions #220
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
It's not an AbstractMatrix after a breaking change in LinearAlgebra: JuliaLang/LinearAlgebra.jl#1097. Also, LQ decomposition in Julia is only consistent for square matrices.
All tests pass, see at https://github.com/aplavin/NamedDims.jl/actions/runs/16301695343/job/46037637119! |
CI had stopped running due to activity. I clicked some buttons... |
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.
Approved if CI passes
for (T, S) in [ | ||
(:NamedDimsVecOrMat, :NamedDimsVecOrMat), | ||
(:NamedDimsVecOrMat, :AbstractVecOrMat), | ||
(:AbstractVecOrMat, :NamedDimsVecOrMat), | ||
for (T, S, U) in [ | ||
(:NamedDimsVecOrMat, :NamedDimsVecOrMat, :AbstractVecOrMat), | ||
(:NamedDimsVecOrMat, :AbstractVecOrMat, :AbstractVecOrMat), | ||
(:AbstractVecOrMat, :NamedDimsVecOrMat, :AbstractVecOrMat), | ||
# needed since Julia 1.10 because of ambiguities introduced by SparseArrays | ||
(:(NamedDimsVecOrMat{<:Any,<:Number}), :(NamedDimsVecOrMat{<:Any,<:Number}), :(AbstractVecOrMat{<:Number})), | ||
(:(NamedDimsVecOrMat{<:Any,<:Number}), :(AbstractVecOrMat{<:Number}), :(AbstractVecOrMat{<:Number})), | ||
(:(AbstractVecOrMat{<:Number}), :(NamedDimsVecOrMat{<:Any,<:Number}), :(AbstractVecOrMat{<:Number})), |
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.
Sad but probably unavoidable
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 didn't check if all entries are needed for the tests to pass (probably no), just repeated the same things as above with <: Number
added.
Idk what the ultimate solution to ambiguities in Julia should be... Note that here in NamedDims ambiguities aren't even related to KeyedArray <-> NamedDims nesting, they are just caused by wanting to overload v/hcat
.
Although maybe overloading v/hcat
is not ultimately necessary?.. Didn't explore it.
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.
Do you know if the 3rd loop is required to pass?
I would love there to be a better way than overloading hcat
. At some point wanted to make Base use axes
everywhere & then perhaps there's a chance you could introduce some pairwise promote
-like scheme. But... for now we hack things to keep working?
I thought there was talk of removing the offending SparseArrays methods but it looks like nothing has happened... JuliaSparse/SparseArrays.jl#431 is the relevant thread.
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.
Which 3rd loop? Do you mean the one with reduce
below?
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.
By 3rd I mean the one over U, instead of cs::AbstractVecOrMat...
before.
There were 3^2 == 9
methods, PR currently makes 6^3 == 216
instead. (And 4096 in mcabbott/AxisKeys.jl#169 ) I don't know that this causes any problems but it does seem like a lot!
Numbers before these PRs:
julia> methods(hcat) |> length # Base & LinearAlgebra
23
julia> using SparseArrays
julia> methods(hcat) |> length
29
julia> using AxisKeys # and NamedDims
julia> methods(hcat) |> length
42
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.
Afaict, it is needed. Note that there's no cartesian product, how do you calculate 216 and 4096?
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'm sorry, you are right. I thought there were 3 nested loops, but clearly there are not.
CI should hopefully pass now!
[ Edit: closes #216 right? ]