Skip to content

fix: xarray roi selection - #672

Merged
tmichela merged 5 commits into
masterfrom
fix/xarray-roi-selection
Oct 29, 2025
Merged

fix: xarray roi selection#672
tmichela merged 5 commits into
masterfrom
fix/xarray-roi-selection

Conversation

@tmichela

@tmichela tmichela commented Oct 20, 2025

Copy link
Copy Markdown
Member

This fixes a few bugs found after #662 was deployed:

For example, with Jungfrau data, shape (1,512,1024) per trainId

  • Ellipsis
    • np.s_[:1,30:40,...]: Returns dim_1 with coords arange(30,40) only. dim_2 has no coords
    • np.s_[...,30:40]: CoordinateValidationError
  • Single valued dimensions are omitted from coords/dimension names
    • np.s_[0,30:40,:]: Returns array with dim_0 arange(30,40), which should be dim_1, retaining dim_0=0 as collapsed dimension
    • np.s_[0, [30, 33], :]: CoordinateValidationError: conflicting sizes for dimension 'dim_0'
  • Related to single valued dimensions, this no longer works:
    • roi = np.s_[0]: returns TypeError: 'int' object is not iterable
    • np.s_[0, 10:20,30:40] with extra_dims=['a', 'b', 'c']: ValueError: different number of dimensions on data and dims: 3 vs 4

Enhancement:
Provides a new extra_coords argument that takes either a dict for user provided coordinates, or a bool to generate default coordinates.

…erest in KeyData.xarray

fix: correctly name default dimensions when an intermediate dimension is dropped.
@tmichela tmichela added the bug Something isn't working label Oct 20, 2025
Comment thread extra_data/tests/test_keydata.py Outdated
Comment thread extra_data/keydata.py Outdated
Comment thread extra_data/keydata.py Outdated
Comment thread extra_data/keydata.py Outdated
@tmichela

Copy link
Copy Markdown
Member Author

@takluyver as discussed offline, we're now adding coordinates if we provide extra_dims argument or if the new extra_coords is True. extra_coords can also be used to provide custom coordinates as a dict.

@tmichela tmichela added the enhancement New feature or request label Oct 27, 2025
Comment thread extra_data/keydata.py Outdated
Comment on lines +40 to +42
# Count non-Ellipsis, non-newaxis dimensions
n_before = sum(1 for x in indexing[:ellipsis_idx] if x is not None)
n_after = sum(1 for x in indexing[ellipsis_idx + 1:] if x is not None)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This can also be simplified now that we're not supporting None

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

updated in edc7554

Comment thread extra_data/keydata.py Outdated
Name the array itself. The default is the source and key joined by a
dot. Ignored for structured data when a dataset is returned.
extra_coords: bool or dict
Add coordinates to the returned DataArray.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

So if you pass both a coordinates dict and a ROI, the coordinates should describe the data after selecting the ROI, if I've understood correctly? I don't have a feeling about which way is better, but it's probably good to document it.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

updated in edc7554

@takluyver

Copy link
Copy Markdown
Member

So just to check I've understood correctly, passing only roi= will no longer attach any coordinates (besides train IDs), and you'll need to use extra_dims or extra_coords to have automatic integer coordinates?

I think this design sounds OK.

@takluyver takluyver added this to the 1.23 milestone Oct 28, 2025
@takluyver

takluyver commented Oct 28, 2025

Copy link
Copy Markdown
Member

In terms of the implementation, I think you can give xarray extra coordinates for dimensions that aren't in the data, without having to do the trick with expanding the array dimensions and then squeezing them back out. Have I misunderstood something? Does that produce a subtly different result?

E.g. attaching a 'z' coordinate with no z dimension:

xr.DataArray(
    np.random.randn(2, 3),
    dims=("x", "y"),
    coords={"x": [10, 20], 'z': 6}
)

- simplify expand_indexing
- improve xarray() docstring for extra_coords option
- simplify handling coordinate for dropped dims
@tmichela

Copy link
Copy Markdown
Member Author

So just to check I've understood correctly, passing only roi= will no longer attach any coordinates (besides train IDs), and you'll need to use extra_dims or extra_coords to have automatic integer coordinates?

Yes, that's exactly that!

@takluyver

Copy link
Copy Markdown
Member

Thanks, LGTM!

@tmichela

Copy link
Copy Markdown
Member Author

Thanks for the review!

@tmichela
tmichela merged commit 9502756 into master Oct 29, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants