-
Notifications
You must be signed in to change notification settings - Fork 41
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
Extending Base.stack for DimArrays #645
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #645 +/- ##
==========================================
+ Coverage 83.83% 83.99% +0.15%
==========================================
Files 45 45
Lines 4102 4136 +34
==========================================
+ Hits 3439 3474 +35
+ Misses 663 662 -1 ☔ View full report in Codecov by Sentry. |
src/array/methods.jl
Outdated
@@ -131,15 +131,15 @@ end | |||
""" | |||
function Base.eachslice(A::AbstractDimArray; dims) | |||
dimtuple = _astuple(dims) | |||
if !(dimtuple == ()) | |||
if !(dimtuple == ()) |
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.
Would be nice to remove all these whitespace changes so we can see the real changes
src/array/methods.jl
Outdated
end | ||
|
||
newdims = first(origdims) | ||
newdims = ntuple(d -> d == newdim ? AnonDim() : newdims[d-(d>newdim)], length(newdims) + 1) |
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.
maybe make this a do
block so its easier to read
src/array/methods.jl
Outdated
|
||
To fix for `AbstractDimArray`, pass new lookup values as `cat(As...; dims=$D(newlookupvals))` keyword or `dims=$D()` for empty `NoLookup`. | ||
""" | ||
|
||
function Base._typed_stack(::Colon, ::Type{T}, ::Type{S}, A, Aax=_iterator_axes(A)) where {T,S<:AbstractDimArray} | ||
origdims = dims.(A) |
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.
origdims = dims.(A) | |
origdims = map(dims, A) |
src/array/methods.jl
Outdated
DimArray(_A, newdims) | ||
end | ||
|
||
function Base.stack(dim::Dimension, A::AbstractVector{<:AbstractDimArray}; dims=nothing, kwargs...) |
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.
Whats the idea with dim
as the first argument?
The tests also don't hit this code.
Thanks for the suggestions! Went through and cleaned up the PR. The Let me know if you have more thoughts on this, happy to work on it further :) |
src/array/methods.jl
Outdated
true | ||
``` | ||
""" | ||
function Base.stack(dim::Dimension, A::AbstractVector{<:AbstractDimArray}; dims=nothing, kwargs...) |
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 not pass dim
in dims
?
This method signature is a bit strange , we usually try not to add varients on Base methods, we just allow dims
to specify named dimensions.
We try not to change base method signatures besides allowing I would just put that new You will also need to |
I see your point about keeping new signatures to a minimum, though. How about allowing the keyword argument
We could also, instead, have that the new dimension is always an |
Using a Always |
src/array/methods.jl
Outdated
true | ||
``` | ||
""" | ||
function Base.stack(A::AbstractVector{<:AbstractDimArray}; dims=Pair(ndims(A[1]), AnonDim()), kwargs...) |
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.
function Base.stack(A::AbstractVector{<:AbstractDimArray}; dims=Pair(ndims(A[1]), AnonDim()), kwargs...) | |
function Base.stack(A::AbstractVector{<:AbstractDimArray}; dims=Pair(ndims(A[1]) + 1, AnonDim()), kwargs...) |
Isn't the default ndims(A) + 1
?
src/array/methods.jl
Outdated
dims isa Integer && (dims = dims => AnonDim()) | ||
dims isa Dimension && (dims = ndims(A[1])+1 => dims) |
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.
dims isa Integer && (dims = dims => AnonDim()) | |
dims isa Dimension && (dims = ndims(A[1])+1 => dims) | |
if dims isa Integer | |
dims = dims => AnonDim()) | |
elseif dims isa Dimension | |
dims = ndims(A[1])+1 => dims) | |
end |
Assigning after &&
is best avoided as the assignment is hard to see when skimming code
src/array/methods.jl
Outdated
end | ||
newdims = format(newdims, B) | ||
|
||
B = rebuild(B; dims=newdims) |
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.
B = rebuild(B; dims=newdims) | |
B = rebuild(B; dims=format(newdims, B)) |
User specified dims are usually incomplete and possibly incorrect
a93d665
to
7a4cf26
Compare
@@ -547,6 +547,100 @@ $message on dimension $D. | |||
To fix for `AbstractDimArray`, pass new lookup values as `cat(As...; dims=$D(newlookupvals))` keyword or `dims=$D()` for empty `NoLookup`. | |||
""" | |||
|
|||
function Base._typed_stack(::Colon, ::Type{T}, ::Type{S}, A, Aax=_iterator_axes(A)) where {T,S<:AbstractDimArray} |
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.
How stable do you think these methods are... could we add a method to Base.stack
instead? What do we gain from touching these internals?
I know we use internals elsewhere, but we should stop:
#522
end | ||
|
||
newdims = first(origdims) | ||
newdims = ntuple(length(newdims) + 1) do d |
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 this type-stable?
Adds methods for
Base.stack
, and related non-exported functions from Base, that are compatible withDimArray
s.Syntax follows Base: stacking
DimArray
s along a given axisdims
creates a new dimension. However, existing dimension data is preserved, and the new dimension becomes anAnonDim
.Optionally, a
Dimension
dim
can be provided as the first argument tostack
, in which case the new dimension is assigned asdim
rather thanAnonDim
.