Closed rafaqz closed 7 months ago
@sethaxen is this close to being ready.
I don't expect a full review as its huge and patchy. But you should be aware of this as it will effect your types.
The two biggest changes for AbstractDimStack
are:
AbstractDimStack
always returns another AbstractDimStack
unless all layers are scalars. Than mean getindex makes fill(x)
layers sometimes. I think you suggested this should be the behaviour in the past!AbstractDimStack
is now correct (I hope), all indexing go via Dimension
wrappers (currently indexing with Int
will map that over all layers, which is fine for same sized stacks but wrong with mixed layers.)Cool! I don't have time to review, but I ran all of my test suites using this PR, and I didn't encounter any new issues.
Thanks! Great they all pass. Mostly the things that broke were awful or buggy anyway and probably not used.
Attention: 104 lines
in your changes are missing coverage. Please review.
Comparison is base (
3316875
) 85.60% compared to head (a85ebf6
) 84.21%.
Files | Patch % | Lines |
---|---|---|
src/dimindices.jl | 80.00% | 24 Missing :warning: |
src/stack/indexing.jl | 65.62% | 22 Missing :warning: |
src/array/indexing.jl | 80.00% | 14 Missing :warning: |
src/Dimensions/primitives.jl | 78.33% | 13 Missing :warning: |
src/LookupArrays/selector.jl | 61.29% | 12 Missing :warning: |
src/stack/methods.jl | 53.84% | 6 Missing :warning: |
src/array/array.jl | 87.87% | 4 Missing :warning: |
src/array/methods.jl | 25.00% | 3 Missing :warning: |
src/stack/stack.jl | 85.00% | 3 Missing :warning: |
src/groupby.jl | 60.00% | 2 Missing :warning: |
... and 1 more |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Major overhaul of indexing to remove bugs and better use all the tools we already have.
AbstractBasicDimArray
was added, andAbstractDimIndices
and other objects likeDimSlices
are now in it along with allAbstractDimArray
. This should give more consistent behavior.CartesianIndex
soDimIndices
can work likeCartesianIndices
DimIndices
can be used to index likeCartesianIndices
but of course it will work accross any dimension permutionsDimKeys
are now calledDimSelectors
(both still exported) because its an array of<:Dimension{<:Selector}
DimSelectors
works just likeDimIndices
. This means you can use it to interpolate, e.g. with aNear
selector.AbstractArray{<:Dimension}
andAbstractArray{<:DimTuple}
work likeAbstractArray{<:CartesianIndex}
but again work on any permutation. They usemergedims
so you still get aDimArray
back.mergedims
now accepts a tuple, and ust combines the dim names into a new dim name, if you want that. Indexing uses this to name dimensions after indexing with an AbstractArray.eachslice
onAbstractDimStack
no uses theDimSlices
object rather than a Generator. TheDimSlices
object is useful forgroupby
as well, so we may as well have foreachslice
too.Breaking:
DimIndices
works for StepRange now and is generally much more likeCartesianIndices
. So it does not hold a LookupArray in its dimensions. Just ranges. This means selectors will not work on it, and should not - its a simple indexing object without lookups.st[i]
is nowst[Dimindices(st)[i]]
AbstractDimStack
propagates for longer in indexing - if not all layers are scalars the scalar layers become zero dimensionalfill(x)
even ingetindex
.