Raise AlignmentError when alignment would reorder an already-aligned index (closes #10714) - #11508
Open
Kayvan-Zahiri wants to merge 2 commits into
Open
Raise AlignmentError when alignment would reorder an already-aligned index (closes #10714)#11508Kayvan-Zahiri wants to merge 2 commits into
Kayvan-Zahiri wants to merge 2 commits into
Conversation
…index An index that requires no reindexing is equal across all objects, so it pins its dimensions in place. It produces no re-indexer, which meant the conflict check in _get_dim_pos_indexers never saw it, and a second index sharing the same dimension could reorder it silently. The mismatched index was then used to combine the data. Record the dimensions pinned by non-reindexed indexes and reject any re-indexer that would reorder one of them. A re-indexer that is the identity still passes, so alignment that only differs on unindexed dimension sizes is unaffected. Closes pydata#10714
for more information, see https://pre-commit.ci
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
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.
Closes #10714.
The bug
alignis documented to raiseAlignmentErrorwhen objects carry conflictingindexes, but the consistency check in
_get_dim_pos_indexersonly runs forindexes that need reindexing:
_need_reindexreturnsFalseprecisely when the matching indexes are alreadyequal, so such an index pins its dimension in place while producing no
re-indexer of its own. Nothing is ever registered for that dimension, so a second
index sharing it has nothing to conflict with and silently wins.
Using @abaarsma's MVCE,
xis identical in both objects andxbis not:It is not only a missing error.
ds2.xbis[4, 6, 5]going in and[4, 5, 6]coming out: the conflicting index is silently rewritten and the mismatched index
is used to combine the data.
align(ds1, ds3), where both indexes needreindexing, raises correctly today, so the difference is purely whether the
second index happened to need reindexing.
The fix
Record the dimensions pinned by non-reindexed indexes, then reject any re-indexer
that would reorder one of them.
The identity check matters and is not redundant.
_need_reindexcan returnTruefor reasons other than index inequality, notably differing unindexed dimension
sizes, and in that case
reindex_likereturns an identity indexer that reordersnothing. Treating "needs reindexing" as "conflicts" would break those cases, so
the guard is specifically "would reorder", not "would reindex".
dim_pos_indexersis unchanged, so the actual reindexing behaviour is untouched.The only difference is that a previously silent case now raises.
Verification
align(ds1, ds3)still raises;align(ds1, ds1)andalign(ds1)are unaffected.test asserts explicitly rather than leaving to inference.
test_indexes.py,test_concat.py,test_merge.py: 300 passed, 2 skipped.aligntest: 1279 passed, 433 skipped, 14 xfailed,4 xpassed. Two failures in
test_chunk_by_season_resamplerare anImportErrorfor
cftimeand reproduce identically on an unmodified checkout, so they areenvironmental rather than caused by this change.
DID NOT RAISEonmainand passes with the fix.Credit
The diagnosis is @abaarsma's, including pinpointing the exact branch in
_get_dim_pos_indexers, and @dcherian confirmed it. Nobody had opened a PR, so Ihave written it up with a regression test and a whatsnew entry.
AI Disclosure
I have tested any AI-generated content in my PR.
I take responsibility for any AI-generated content in my PR.
Tools: Claude (Claude Code), used to diagnose the root cause, draft the fix and the
regression test, and draft this description.