Skip to content

Refactor RasterIndex internals (do not wrap PandasIndex)#43

Merged
dcherian merged 13 commits intoxarray-contrib:mainfrom
benbovy:remove-wrapped-pandasindex
Jun 27, 2025
Merged

Refactor RasterIndex internals (do not wrap PandasIndex)#43
dcherian merged 13 commits intoxarray-contrib:mainfrom
benbovy:remove-wrapped-pandasindex

Conversation

@benbovy
Copy link
Copy Markdown
Member

@benbovy benbovy commented Jun 26, 2025

Closes #40.

This PR limits RasterIndex._wrapped_indexes to these two exact cases:

  • {("x", "y"): CoordinateTransformIndex}
  • {"x": AxisAffineTransformIndex, "y": AxisAffineTransformIndex}

This makes things much easier.

@benbovy benbovy requested a review from dcherian June 26, 2025 12:11
@benbovy
Copy link
Copy Markdown
Member Author

benbovy commented Jun 26, 2025

Considering only these two cases, using a dictionary to store the wrapped indexes is overkill. This seems much cleaner:

class RasterIndex(Index):
    _index: CoordinateTransformIndex | tuple[AxisAffineTransformIndex, AxisAffineTransformIndex]

# or 

class RasterIndex(Index):
    _index: CoordinateTransformIndex | None
    _xy_indexes: tuple[AxisAffineTransformIndex, AxisAffineTransformIndex] | None

Storing coordinate names in RasterIndex._wrapped_indexes is also confusing and error-prone. We can retrieve the coordinate and dimension names of the X/Y axes from the CoordinateTransform objects wrapped by the underlying indexes.

@benbovy benbovy changed the title Do not wrap PandasIndex in RasterIndex anymore Refactor RasterIndex internals (do not wrap PandasIndex) Jun 26, 2025
Comment thread src/rasterix/raster_index.py Outdated
Comment thread src/rasterix/raster_index.py Outdated
@benbovy benbovy mentioned this pull request Jun 27, 2025
@benbovy
Copy link
Copy Markdown
Member Author

benbovy commented Jun 27, 2025

Ready for review!

benbovy added 3 commits June 27, 2025 16:09
In the end I find this approach a bit more cleaner / clearer.
Comment thread src/rasterix/raster_index.py Outdated
Copy link
Copy Markdown
Contributor

@dcherian dcherian left a comment

Choose a reason for hiding this comment

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

thanks! LGTM. w

Co-authored-by: Deepak Cherian <dcherian@users.noreply.github.com>
@dcherian
Copy link
Copy Markdown
Contributor

Merging to make progress on docs.

@dcherian dcherian merged commit 1ca47c5 into xarray-contrib:main Jun 27, 2025
6 checks passed
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.

Preserve scalar index?

2 participants