Extending Base.stack for DimArrays #645

brendanjohnharris · 2024-02-23T06:29:57Z

Adds methods for Base.stack, and related non-exported functions from Base, that are compatible with DimArrays.

Syntax follows Base: stacking DimArrays along a given axis dims creates a new dimension. However, existing dimension data is preserved, and the new dimension becomes an AnonDim.

Optionally, a Dimension dim can be provided as the first argument to stack, in which case the new dimension is assigned as dim rather than AnonDim.

codecov-commenter · 2024-02-23T06:43:09Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.99%. Comparing base (9846bfe) to head (884ab68).
Report is 41 commits behind head on main.

❗ Current head 884ab68 differs from pull request most recent head 2c7b8b9

Please upload reports for the commit 2c7b8b9 to get more accurate results.

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.
📢 Have feedback on the report? Share it here.

rafaqz · 2024-02-23T09:03:13Z

src/array/methods.jl

@@ -131,15 +131,15 @@ end
    """
    function Base.eachslice(A::AbstractDimArray; dims)
        dimtuple = _astuple(dims)
-        if !(dimtuple == ()) 
+        if !(dimtuple == ())


Would be nice to remove all these whitespace changes so we can see the real changes

rafaqz · 2024-02-23T09:06:25Z

src/array/methods.jl

+    end
+
+    newdims = first(origdims)
+    newdims = ntuple(d -> d == newdim ? AnonDim() : newdims[d-(d>newdim)], length(newdims) + 1)


maybe make this a do block so its easier to read

rafaqz · 2024-02-23T09:08:11Z

src/array/methods.jl


 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)


Suggested change

origdims = dims.(A)

origdims = map(dims, A)

rafaqz · 2024-02-23T09:10:56Z

src/array/methods.jl

+    DimArray(_A, newdims)
+end
+
+function Base.stack(dim::Dimension, A::AbstractVector{<:AbstractDimArray}; dims=nothing, kwargs...)


Whats the idea with dim as the first argument?

The tests also don't hit this code.

brendanjohnharris · 2024-02-27T05:00:45Z

Thanks for the suggestions! Went through and cleaned up the PR.

The Base.stack(dim, X; dims) method lets you supply a Dimension to assign to the newly created array dimension; not strictly necessary, but more convenient than setting or rebuilding the resulting array to overwrite the new AnonDim (is an AnonDim a suitable way to treat a newly created dimension, as I've done here?).

Let me know if you have more thoughts on this, happy to work on it further :)

rafaqz · 2024-02-27T07:24:27Z

src/array/methods.jl

+true
+```
+"""
+function Base.stack(dim::Dimension, A::AbstractVector{<:AbstractDimArray}; dims=nothing, kwargs...)


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.

rafaqz · 2024-02-27T07:32:43Z

We try not to change base method signatures besides allowing dims to be named. To keep the mental model very simple.

I would just put that new Dimension in dims like cat does. If its a constructed dimension it replaces the existing one. Otherwise Type/Symbol etc just choose them. Just check d isa Dimension

You will also need to format(newdims, A) afterwards.

brendanjohnharris · 2024-02-28T03:08:54Z

Base.stack behaves differently than Base.cat in that it always creates a new dimension (i.e. Base.stack([A, B,...]; dims=2) inserts a dimension at position 2, increasing ndims(A) by 1); in general this new dimension is unrelated to the existing array dimensions, so we need to specify both position (dims) and a Dimension (dim).

I see your point about keeping new signatures to a minimum, though. How about allowing the keyword argument dims to be assigned as an 'Integer=>Dimension' pair, with the following behavior:

If dims isa Integer: The new dimension is an AnonDim at position dims
If dims isa Dimension: The new dimension is a DImension dims at position ndims(A)+1
If dims isa Pair{Integer, Dimension}: The new dimension is a Dimension last(dims) at position first(dims)

We could also, instead, have that the new dimension is always an AnonDim, leaving it up to the user to set that dimension at a later time; in that case, however, there is ambiguity if there are any existing AnonDims in the input arrays.

rafaqz · 2024-02-28T21:21:45Z

Using a Pair sounds like a good solution (as do points 1 and 2 as simpler cases). For 2 I guess putting it as the last dimension would be the majority use-case? (I rarely use stack)

Always AnonDim will require using set all the time with AnonDim => somedim. We may as well just put that pair in stack and skip the step.

rafaqz · 2024-03-03T12:13:45Z

src/array/methods.jl

+true
+```
+"""
+function Base.stack(A::AbstractVector{<:AbstractDimArray}; dims=Pair(ndims(A[1]), AnonDim()), kwargs...)


Suggested change

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 ?

rafaqz · 2024-03-03T12:16:53Z

src/array/methods.jl

+    dims isa Integer && (dims = dims => AnonDim())
+    dims isa Dimension && (dims = ndims(A[1])+1 => dims)


Suggested change

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

rafaqz · 2024-03-03T12:24:26Z

src/array/methods.jl

+        end
+        newdims = format(newdims, B)
+
+        B = rebuild(B; dims=newdims)


Suggested change

B = rebuild(B; dims=newdims)

B = rebuild(B; dims=format(newdims, B))

User specified dims are usually incomplete and possibly incorrect

rafaqz · 2024-03-09T21:10:51Z

src/array/methods.jl

@@ -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}


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

rafaqz · 2024-03-09T21:13:22Z

src/array/methods.jl

+    end
+
+    newdims = first(origdims)
+    newdims = ntuple(length(newdims) + 1) do d


Is this type-stable?

src/array/methods.jl

brendanjohnharris added 2 commits February 23, 2024 17:27

Extend Base.stack to DimArrays

4ea5e40

Initial Base.stack tests

ec3b2cf

brendanjohnharris mentioned this pull request Feb 23, 2024

Piracy! brendanjohnharris/TimeseriesTools.jl#10

Closed

rafaqz reviewed Feb 23, 2024

View reviewed changes

brendanjohnharris added 4 commits February 24, 2024 00:01

Fix whitespace changes

0b762ce

Merge branch 'stack' into stacker

6affd9b

Clean up Base.stack, add docstrings, add tests

ac3affc

Fix Base.stack docstring

83ace60

brendanjohnharris marked this pull request as ready for review February 27, 2024 05:01

rafaqz reviewed Feb 27, 2024

View reviewed changes

New Pair syntax for extended Base.stack

e6cfb1b

rafaqz reviewed Mar 3, 2024

View reviewed changes

brendanjohnharris and others added 3 commits March 7, 2024 14:31

Better Base.stack formatting

ce16068

Tweak docstring for Base.stack, remove kwargs

bdf151f

Merge branch 'rafaqz:main' into stack

7a4cf26

brendanjohnharris force-pushed the stack branch 2 times, most recently from a93d665 to 7a4cf26 Compare March 7, 2024 05:01

brendanjohnharris added 3 commits March 7, 2024 16:10

Update whitespace

3314852

Fix whitespace

0ac2eb0

Fix whitespace

884ab68

rafaqz reviewed Mar 9, 2024

View reviewed changes

src/array/methods.jl

end

newdims = first(origdims)

newdims = ntuple(length(newdims) + 1) do d

Copy link

Owner

rafaqz Mar 9, 2024

Choose a reason for hiding this comment

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

Is this type-stable?

rafaqz reviewed Mar 9, 2024

View reviewed changes

src/array/methods.jl Outdated Show resolved Hide resolved

rafaqz reviewed Mar 9, 2024

View reviewed changes

src/array/methods.jl Outdated Show resolved Hide resolved

rafaqz added 2 commits May 19, 2024 09:58

use rebuild

c369d06

rebuild

2c7b8b9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extending Base.stack for DimArrays #645

Extending Base.stack for DimArrays #645

brendanjohnharris commented Feb 23, 2024 •

edited

Loading

codecov-commenter commented Feb 23, 2024 •

edited by codecov bot

Loading

rafaqz Feb 23, 2024

rafaqz Feb 23, 2024

rafaqz Feb 23, 2024

rafaqz Feb 23, 2024

brendanjohnharris commented Feb 27, 2024

rafaqz Feb 27, 2024

rafaqz commented Feb 27, 2024

brendanjohnharris commented Feb 28, 2024

rafaqz commented Feb 28, 2024 •

edited

Loading

rafaqz Mar 3, 2024 •

edited

Loading

rafaqz Mar 3, 2024

rafaqz Mar 3, 2024

rafaqz Mar 9, 2024 •

edited

Loading

rafaqz Mar 9, 2024

	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...)

		dims isa Integer && (dims = dims => AnonDim())
		dims isa Dimension && (dims = ndims(A[1])+1 => dims)

	B = rebuild(B; dims=newdims)
	B = rebuild(B; dims=format(newdims, B))

Extending Base.stack for DimArrays #645

Are you sure you want to change the base?

Extending Base.stack for DimArrays #645

Conversation

brendanjohnharris commented Feb 23, 2024 • edited Loading

codecov-commenter commented Feb 23, 2024 • edited by codecov bot Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendanjohnharris commented Feb 27, 2024

Choose a reason for hiding this comment

rafaqz commented Feb 27, 2024

brendanjohnharris commented Feb 28, 2024

rafaqz commented Feb 28, 2024 • edited Loading

rafaqz Mar 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafaqz Mar 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendanjohnharris commented Feb 23, 2024 •

edited

Loading

codecov-commenter commented Feb 23, 2024 •

edited by codecov bot

Loading

rafaqz commented Feb 28, 2024 •

edited

Loading

rafaqz Mar 3, 2024 •

edited

Loading

rafaqz Mar 9, 2024 •

edited

Loading