fixes and updates for Julia 1.9 #130
Conversation
aplavin
commented
Sep 6, 2022
aplavin
commented
- add new: Base.stack for keyed arrays
- update to match 1.9: Base.uncolon and Base.eachslice
|
some tests still fail on nightly |
| @static if VERSION > v"1.9-DEV" | ||
| function Base.eachslice(A::KeyedArray; dims) | ||
| dims_ix = AxisKeys.dim(A, dims) |> Tuple | ||
| data = @invoke eachslice(A::AbstractArray; dims=dims_ix) |
There was a problem hiding this comment.
Rather than @invoke, could this just call the next function down? There seems to be an _eachslice which is always called, like so:
@inline function eachslice(A; dims, drop=true)
_eachslice(A, dims, drop)
end
eachrow(A::AbstractMatrix) = _eachslice(A, (1,), true)
eachcol(A::AbstractMatrix) = _eachslice(A, (2,), true)
There was a problem hiding this comment.
Instead of depending on internals, I think either the current @invoke solution or something like eachslice(keyless_unname(A); dims) are cleaner. No preference between the two.
There was a problem hiding this comment.
The keyless_unname way may require some processing for inner arrays though.
| elseif VERSION >= v"1.1" | ||
| # This copies the implementation from Base, except with numerical_dims: | ||
| @inline function Base.eachslice(A::KeyedArray; dims) |
There was a problem hiding this comment.
JuliaLang/Compat.jl#663 still isn't merged, so it makes sense to keep this.
There was a problem hiding this comment.
Not sure if I follow. How is Compat related here? This PR only makes changes on 1.9+.
src/functions.jl
Outdated
| @static if VERSION > v"1.9-DEV" | ||
| function Base.stack(A::KeyedArray; dims::Colon=:) | ||
| data = @invoke Base.stack(A::AbstractArray; dims) |
There was a problem hiding this comment.
We should consider just depending on Compat.jl for this. Compat = "3.46, 4.2"
Here too there's an internal method we could call after dispatch, instead of @invoke:
stack(iter; dims=:) = _stack(dims, iter)
There was a problem hiding this comment.
There is also stack.jl BTW:
Line 7 in cddf60f
[email protected] is much simplified, to use Base.stack where possible. Some of the methods overloaded there don't exist anymore. No pressure though to sort it all out, that version is presently marked incompatible.
But perhaps stack needs to be qualified in the tests, to avoid some failures?
There was a problem hiding this comment.
We should consider just depending on Compat.jl for this.
Maybe, no preference on my side.
stack(iter; dims=:) = _stack(dims, iter)
As above, I find calling the actual public API functions cleaner than some internal stuff. Here keyless_unname should be a drop-in replacement for @invoke, if that's preferred.
There was a problem hiding this comment.
I deliberately didn't change anything regarding LazyStack.stack. Didn't use that package, and not sure about how it interplays with Base stack and everything else.
|
With new commits, everything should pass on both nightly and older versions. |
|
CanonicalIndexError should be fixed with JuliaLang/julia#47008 |
|
Julia 1.8.3 indeed fixes that bug, and all tests now pass with no changes from my side. Anything else I need to modify in this PR? |
|
Bump @mcabbott |
|
Another bump... @mcabbott |
|
I'm using the package in my tests and am getting ready for 1.9... but it fails due to the issues addressed in this PR would be great to have it merged @mcabbott |
|
Bump. It would be good to get this one in and released such that we can continue the testing of Julia 1.9 |
