Skip to content

Enable individual indexing for merged lookups#980

Merged
asinghvi17 merged 9 commits intomainfrom
as/individualindexing
Apr 27, 2025
Merged

Enable individual indexing for merged lookups#980
asinghvi17 merged 9 commits intomainfrom
as/individualindexing

Conversation

@asinghvi17
Copy link
Copy Markdown
Collaborator

ds = rand(X(1:10), Y(1:10), Ti(1:10))
m = mergedims(ds, (X, Y) => :space)

m[X(At(1))]

previously threw an error, with this PR it just works

What this does is that it introduces a new AbstractMergedLookup supertype that requires that the lookup have dims. this way we can find all lookups that might be secretly multidimensional (currently just merged lookup and geometry lookup) and pass all matching dims into them directly. That gets us back a vector index like thing that we can replace the placeholder Colon with that we receive from the split_dims.

This enables geometry lookups in Rasters to work like this as well, and also selecting by extents, crop, etc.

This isn't actually breaking in any way so can be merged for a non breaking release, I think.

Needs tests and benchmarks to check that this does not impact the getindex time, ie it should just compile away

Comment thread src/Dimensions/indexing.jl Outdated
Comment thread src/Dimensions/indexing.jl Outdated
Comment thread src/Dimensions/merged.jl Outdated
@rafaqz
Copy link
Copy Markdown
Owner

rafaqz commented Apr 23, 2025

Had a thought: what happens if e.g. X and and Y are in the regular dimensions, and also in the merged dimensions

@asinghvi17
Copy link
Copy Markdown
Collaborator Author

asinghvi17 commented Apr 23, 2025

In the current implementation the merged dimensions are ignored, because X and Y don't make it out of otherdims

We could in theory duplicate those. But it seems fragile and pretty ambiguous if your array is already fully specified.

@rafaqz
Copy link
Copy Markdown
Owner

rafaqz commented Apr 23, 2025

Yeah, Its probably best. I just wondered about the behaviour where there are two merged dims and X/Y working on both of them, but not if there is also regular X/Y. That may be slightly confusing.

@codecov
Copy link
Copy Markdown

codecov bot commented Apr 23, 2025

Codecov Report

Attention: Patch coverage is 33.33333% with 18 lines in your changes missing coverage. Please review.

Project coverage is 75.81%. Comparing base (20b42df) to head (4c3d727).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/Dimensions/dimension.jl 0.00% 17 Missing ⚠️
src/Dimensions/indexing.jl 88.88% 1 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (20b42df) and HEAD (4c3d727). Click for more details.

HEAD has 5 uploads less than BASE
Flag BASE (20b42df) HEAD (4c3d727)
6 1
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #980      +/-   ##
==========================================
- Coverage   83.44%   75.81%   -7.64%     
==========================================
  Files          54       54              
  Lines        5154     5168      +14     
==========================================
- Hits         4301     3918     -383     
- Misses        853     1250     +397     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@rafaqz
Copy link
Copy Markdown
Owner

rafaqz commented Apr 24, 2025

I think the MergedLookup docstring need indexing examples so this isn't hidden

@asinghvi17 asinghvi17 requested a review from rafaqz April 25, 2025 02:07
@asinghvi17
Copy link
Copy Markdown
Collaborator Author

ok, all relevant tests are passing.

no clue why plots is failing though..are you using some strange backend?

Comment thread src/Dimensions/indexing.jl Outdated
@rafaqz
Copy link
Copy Markdown
Owner

rafaqz commented Apr 25, 2025

It's just GR. We can ignore it for now

@asinghvi17 asinghvi17 merged commit 6c4cd33 into main Apr 27, 2025
2 of 10 checks passed
@asinghvi17 asinghvi17 deleted the as/individualindexing branch April 27, 2025 15:15
@asinghvi17
Copy link
Copy Markdown
Collaborator Author


julia> using DimensionalData
imp
julia> import DimensionalData as DD

julia> using DimensionalData.Dimensions: _experimental_extent

julia> using Extents

julia> d = rand(X(1:10), Y(1:10), Z(1:10), Ti(1:10), Dim{:something}(1:2))
┌ 10×10×10×10×2 DimArray{Float64, 5} ┐
├────────────────────────────────────┴────────────────────────────────────────────────────────────────────────────────────────────────── dims ┐
  ↓ X         Sampled{Int64} 1:10 ForwardOrdered Regular Points,
  → Y         Sampled{Int64} 1:10 ForwardOrdered Regular Points,
  ↗ Z         Sampled{Int64} 1:10 ForwardOrdered Regular Points,
  ⬔ Ti        Sampled{Int64} 1:10 ForwardOrdered Regular Points,
  ◩ something Sampled{Int64} 1:2 ForwardOrdered Regular Points
└─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘
[:, :, 1, 1, 1]
 ⋮      ⋱  

julia> dm = mergedims(d, (X, Y) => :space, (Z, Ti) => :valu)
┌ 2×100×100 DimArray{Float64, 3} ┐
├────────────────────────────────┴────────────────────────────────────────────────────────────────────────────────────────────────────── dims ┐
  ↓ something Sampled{Int64} 1:2 ForwardOrdered Regular Points,
  → space     MergedLookup{Tuple{Int64, Int64}} [(1, 1), (2, 1), …, (9, 10), (10, 10)] d↓ X, → Y,
  ↗ dvalu      MergedLookup{Tuple{Int64, Int64}} [(1, 1), (2, 1), …, (9, 10), (10, 10)] ↓ Z, → Ti
└─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘
[:, :, 1]
 ⋮      ⋱  

julia> ddm = dims(dm)
(↓ something Sampled{Int64} 1:2 ForwardOrdered Regular Points,
→ space     MergedLookup{Tuple{Int64, Int64}} [(1, 1), (2, 1), …, (9, 10), (10, 10)] (↓ X, → Y),
↗ valu      MergedLookup{Tuple{Int64, Int64}} [(1, 1), (2, 1), …, (9, 10), (10, 10)] (↓ Z, → Ti))

felixcremer pushed a commit to felixcremer/DimensionalData.jl that referenced this pull request Jul 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants