Alignment of n-dimensional indexes with partially excluded dims #10293
+175
−27
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Support checking exact alignment of indexes that use multiple dimensions in the cases where some of those dimensions are included in alignment while others are excluded. Added `exclude_dims` keyword argument to `Index.equals()` (and still support old signature with future warning). Also fixed bug: indexes associated with scalar coordinates were ignored during alignment. Added tests as well.
benbovy
commented
May 7, 2025
Comment on lines
+352
to
+360
| @overload | ||
| def equals(self, other: Index) -> bool: ... | ||
|
|
||
| @overload | ||
| def equals( | ||
| self, other: Index, *, exclude_dims: frozenset[Hashable] | None = None | ||
| ) -> bool: ... | ||
|
|
||
| def equals(self, other: Index, **kwargs) -> bool: |
There was a problem hiding this comment.
I added those overloads so Mypy doesn't fail for 3rd-party Xarray indexes. But maybe we want Mypy to fail and thereby encourage maintainers updating their code?
benbovy
commented
May 7, 2025
Comment on lines
+369
to
+378
| exclude_dims : frozenset of hashable, optional | ||
| All the dimensions that are excluded from alignment, or None by default | ||
| (i.e., when this method is not called in the context of alignment). | ||
| For a n-dimensional index it allows ignoring any relevant dimension found | ||
| in ``exclude_dims`` when comparing this index with the other index. | ||
| For a 1-dimensional index it can be always safely ignored as this | ||
| method is not called when all of the index's dimensions are also excluded | ||
| from alignment | ||
| (note: the index's dimensions correspond to the union of the dimensions | ||
| of all coordinate variables associated with this index). |
There was a problem hiding this comment.
Could probably be improved a bit.
dcherian
reviewed
May 8, 2025
Comment on lines
+370
to
+378
| All the dimensions that are excluded from alignment, or None by default | ||
| (i.e., when this method is not called in the context of alignment). | ||
| For a n-dimensional index it allows ignoring any relevant dimension found | ||
| in ``exclude_dims`` when comparing this index with the other index. | ||
| For a 1-dimensional index it can be always safely ignored as this | ||
| method is not called when all of the index's dimensions are also excluded | ||
| from alignment | ||
| (note: the index's dimensions correspond to the union of the dimensions | ||
| of all coordinate variables associated with this index). |
There was a problem hiding this comment.
Suggested change
| All the dimensions that are excluded from alignment, or None by default | |
| (i.e., when this method is not called in the context of alignment). | |
| For a n-dimensional index it allows ignoring any relevant dimension found | |
| in ``exclude_dims`` when comparing this index with the other index. | |
| For a 1-dimensional index it can be always safely ignored as this | |
| method is not called when all of the index's dimensions are also excluded | |
| from alignment | |
| (note: the index's dimensions correspond to the union of the dimensions | |
| of all coordinate variables associated with this index). | |
| Dimensions excluded from checking. It is None by default, | |
| (i.e., when this method is not called in the context of alignment). | |
| For a n-dimensional index this option allows an Index to optionally, | |
| ignore any dimension in ``exclude_dims`` when comparing | |
| ``self`` with ``other``. For a 1-dimensional index this kwarg can be safely ignored , | |
| as this method is not called when all of the index's dimensions are also excluded | |
| from alignment (note: the index's dimensions correspond to the union of the dimensions | |
| of all coordinate variables associated with this index). |
dcherian
reviewed
May 8, 2025
| f"the signature ``{index_cls_name}.equals(self, other)`` is deprecated. " | ||
| f"Please update it to " | ||
| f"``{index_cls_name}.equals(self, other, *, exclude_dims=None)`` " | ||
| "or kindly ask the maintainers doing it. " |
There was a problem hiding this comment.
Suggested change
| "or kindly ask the maintainers doing it. " | |
| "or kindly ask the maintainers of ``{index_cls_name}`` to do it. " |
otherwise they might come to us ;)
dcherian
reviewed
May 8, 2025
| @@ -216,30 +216,37 @@ def _normalize_indexes( | |||
|
|
|||
| normalized_indexes = {} | |||
There was a problem hiding this comment.
Can we add a comment describing what "normalize" means here please? it would also be good to describe the purpose of each loop with a block comment. I can't figure out what's going on here
dcherian
reviewed
May 8, 2025
| @@ -863,7 +882,7 @@ def sel( | |||
|
|
|||
| return IndexSelResult({self.dim: indexer}) | |||
|
|
|||
| def equals(self, other: Index): | |||
| def equals(self, other: Index, *, exclude_dims: frozenset[Hashable] | None = None): | |||
There was a problem hiding this comment.
Suggested change
| def equals(self, other: Index, *, exclude_dims: frozenset[Hashable] | None = None): | |
| def equals(self, other: Index, *, exclude: frozenset[Hashable] | None = None): |
align calls this exclude. Shall we keep that name for consistency?
dcherian
reviewed
May 8, 2025
| exclude_dims = idx_all_dims & self.exclude_dims | ||
| if exclude_dims == idx_all_dims: | ||
| continue | ||
| elif exclude_dims and self.join != "exact": |
There was a problem hiding this comment.
I'm having trouble deciding if any of the other cases make any sense. Seems OK to remove anything that requires actual reindexing, which leaves "override" and I'm not sure about that. Let's have the error message recommend the user to open an issue if they feel it should work.
dcherian
approved these changes
May 8, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
whats-new.rstSupport checking exact alignment of indexes that use multiple dimensions in the cases where some of those dimensions are included in alignment while others are excluded.
Add the
exclude_dimskeyword argument toIndex.equals()(still support old signature withFutureWarning).Also fixed a bug: indexes associated with scalar coordinates were ignored during alignment.
TODO:
exclude_dimskwarg toCoordinateTransform.equals()as welljoin='exact'copy or assign the index of each original object in the new aligned objects instead of assigning the aligned index (when copying this may have an impact on performance).