Skip to content
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

Open
wants to merge 30 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
4ea5e40
Extend Base.stack to DimArrays
brendanjohnharris Feb 23, 2024
ec3b2cf
Initial Base.stack tests
brendanjohnharris Feb 23, 2024
0b762ce
Fix whitespace changes
brendanjohnharris Feb 23, 2024
6affd9b
Merge branch 'stack' into stacker
brendanjohnharris Feb 23, 2024
ac3affc
Clean up Base.stack, add docstrings, add tests
brendanjohnharris Feb 27, 2024
83ace60
Fix Base.stack docstring
brendanjohnharris Feb 27, 2024
e6cfb1b
New Pair syntax for extended Base.stack
brendanjohnharris Mar 1, 2024
ce16068
Better Base.stack formatting
brendanjohnharris Mar 7, 2024
bdf151f
Tweak docstring for Base.stack, remove kwargs
brendanjohnharris Mar 7, 2024
7a4cf26
Merge branch 'rafaqz:main' into stack
brendanjohnharris Mar 7, 2024
3314852
Update whitespace
brendanjohnharris Mar 7, 2024
0ac2eb0
Fix whitespace
brendanjohnharris Mar 7, 2024
884ab68
Fix whitespace
brendanjohnharris Mar 7, 2024
c369d06
use rebuild
rafaqz May 19, 2024
2c7b8b9
rebuild
rafaqz May 19, 2024
cf57f86
Merge branch 'rafaqz:main' into stack
brendanjohnharris Nov 15, 2024
0a278a6
Update Base.stack tests
brendanjohnharris Nov 15, 2024
f41f424
Rework Base.stack
brendanjohnharris Nov 15, 2024
d9ee5e7
Update Base.stack to accept dims<:Dimension
brendanjohnharris Nov 17, 2024
9b3b3b7
Improve Base.stack tests
brendanjohnharris Nov 17, 2024
25dc8a2
Add test for Base.stack with symbol dims
brendanjohnharris Nov 17, 2024
e2aeb0e
Replace accidentally deleted line for Base.stack
brendanjohnharris Nov 17, 2024
39c78e8
Update Base.stack for symbol dims
brendanjohnharris Nov 17, 2024
16c205a
Fix typo
brendanjohnharris Nov 17, 2024
15cc9fd
Avoid dispatching on Base._stack
brendanjohnharris Nov 19, 2024
52ce34a
Update tests
brendanjohnharris Nov 19, 2024
f868ddf
Update test with error on out-of-range dims
brendanjohnharris Nov 19, 2024
911acda
Update _maybe_dimnum to error if specified Integer dims are too large
brendanjohnharris Nov 19, 2024
9517d16
Use map instead of dot broadcast
brendanjohnharris Nov 21, 2024
b329519
Update reference.md
brendanjohnharris Nov 21, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 80 additions & 0 deletions src/array/methods.jl
Original file line number Diff line number Diff line change
Expand Up @@ -547,6 +547,86 @@ $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}
Copy link
Owner

@rafaqz rafaqz Mar 9, 2024

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

origdims = map(dims, A)
_A = parent.(A)
t = eltype(_A)
_A = Base._typed_stack(:, T, t, A)

if !comparedims(Bool, origdims...;
order=true, val=true, warn=" Can't `stack` AbstractDimArray, applying to `parent` object."
)
return _A
else
DimArray(_A, (first(origdims)..., AnonDim()))
rafaqz marked this conversation as resolved.
Show resolved Hide resolved
end
end

function Base._dim_stack(newdim::Integer, ::Type{T}, ::Type{S}, A) where {T,S<:AbstractDimArray}
origdims = dims.(A)
_A = parent.(A)
t = eltype(_A)
_A = Base._dim_stack(newdim, T, t, A)

if !comparedims(Bool, origdims...;
order=true, val=true, warn=" Can't `stack` AbstractDimArray, applying to `parent` object."
)
return _A
end

newdims = first(origdims)
newdims = ntuple(length(newdims) + 1) do d
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this type-stable?

if d == newdim
AnonDim()
else # Return the old dimension, shifted across once if it comes after the new dim
newdims[d-(d>newdim)]
end
end
DimArray(_A, newdims)
rafaqz marked this conversation as resolved.
Show resolved Hide resolved
end

"""
Base.stack([dim::Dimension], A::AbstractVector{<:AbstractDimArray}; dims=nothing)

Stack arrays along a specified axis `dims`, while preserving the dimensional
information of other axes.
If the optional `Dimension` argument `dim` is supplied, it must have
`length(dim) == length(A)`; the resulting axis `dims` is given the dimension `dim`.
If `dim` is not supplied, the new dimension will be an `AnonDim`.

# Examples
```julia-repl
julia> da = DimArray([1 2 3; 4 5 6], (X(10:10:20), Y(300:-100:100)));
julia> db = DimArray([6 5 4; 3 2 1], (X(10:10:20), Y(300:-100:100)));

# Stack along a new dimension `Z`
julia> dc = stack(Z(1:2), [da, db], dims=3)
╭─────────────────────────╮
│ 2×3×2 DimArray{Int64,3} │
├─────────────────────────┴──────────────────────────────── dims ┐
↓ X Sampled{Int64} 10:10:20 ForwardOrdered Regular Points,
→ Y Sampled{Int64} 300:-100:100 ReverseOrdered Regular Points,
↗ Z 1:2
└────────────────────────────────────────────────────────────────┘

julia> dims(dc, 3) == Z(1:2)
true
julia> parent(dc) == stack(map(parent, [da, db]), dims=3)
true
```
"""
function Base.stack(dim::Dimension, A::AbstractVector{<:AbstractDimArray}; dims=nothing, kwargs...)
Copy link
Owner

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.

Copy link
Owner

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.

B = Base.stack(A; dims, kwargs...)
newdims = ntuple(ndims(B)) do d
if d == dims # Use the new provided dimension
dim
else
DimensionalData.dims(B, d)
end
end
rebuild(B; dims=newdims)
end

function Base.inv(A::AbstractDimArray{T,2}) where T
newdata = inv(parent(A))
newdims = reverse(dims(A))
Expand Down
20 changes: 20 additions & 0 deletions test/methods.jl
Original file line number Diff line number Diff line change
Expand Up @@ -655,6 +655,26 @@ end
end
end

@testset "Base.stack" begin
a = [1 2 3; 4 5 6]
da = DimArray(a, (X(4.0:5.0), Y(6.0:8.0)))
b = [7 8 9; 10 11 12]
ca = DimArray(b, (X(4.0:5.0), Y(6.0:8.0)))
db = DimArray(b, (X(6.0:7.0), Y(6.0:8.0)))

@test stack([da, db]; dims=3) == stack([parent(da), parent(db)], dims=3)
@test_warn "Lookup values for X" stack([da, db]; dims=3)

@test stack([da, ca]; dims=1) == stack([parent(da), parent(ca)], dims=1)
@test_warn "Lookup values for X" stack([da, db]; dims=1)

for d = 1:3
dc = stack(Z(1:2), [da, ca], dims=d)
@test dims(dc, d) == Z(1:2)
@test parent(dc) == stack(map(parent, [da, db]), dims=d)
end
end

@testset "unique" begin
a = [1 1 6; 1 1 6]
xs = (X, X(), :X)
Expand Down
Loading