-
Notifications
You must be signed in to change notification settings - Fork 13
Detect out-of-range integer indices in __setitem__ operations
#378
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
sadielbartholomew
merged 14 commits into
NCAS-CMS:main
from
sadielbartholomew:out-of-range-indexing
Jan 15, 2026
Merged
Changes from all commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
8724f07
Add test for IndexError on out-of-range indices for getitem
sadielbartholomew 4519c28
Add test for IndexError on out-of-range indices for setitem
sadielbartholomew e0254e7
Update functions.parse_indices with error for out-of-range indices
sadielbartholomew 7b43067
Update functions.parse_indices clarification on out-of-range cases
sadielbartholomew a405feb
Update changelog for PR for closing #377
sadielbartholomew f0c286c
Update functions.parse_indices to move logic into similar loop
sadielbartholomew 0840aeb
Merge branch 'main' into out-of-range-indexing
sadielbartholomew bab3de0
Update tests of out-of-range indexing for negative indices cases
sadielbartholomew dabacc5
Update out-of-range indexing error for negative indices cases
sadielbartholomew bee1291
Apply linters on touched code for PR #378
sadielbartholomew bb966aa
Update cfdm/functions.py by consolidating parse_indices conditionals
sadielbartholomew 486970d
Revert "Update cfdm/functions.py by consolidating..."
sadielbartholomew 2b8a461
Update functions.parse_indices to avoid duplicate isinstance call
sadielbartholomew f852465
Merge branch 'main' into out-of-range-indexing
sadielbartholomew File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need some indentation here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(my fault for not including it in my last suggestion!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AH - I made a comment about this on the thread and now I don't see it. Not sure why it's disappeared!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK after checking my tabs said comment is gone with no trace - did I just imagine/dream posting it?
What I said was (in said dream, or otherwise) is that there is an
elif hasattr(index, "to_dask_array"):following the originalif keepdims and isinstance(index, Integral):which means that converting to having the top-levelif isinstance(index, Integral):actually complicates the flow - we'd need further somewhat contrived checking afterwards to get the equivalent. So actually I am not sure it's a good idea.That said, no need to actually call
isinstancetwice - so I suggest we store that in a variable to use each time but use the original conditional structure. What do you think?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GitHub having a moment ...
Thanks for spotting this. I think what you say is worthwhile. It may seem like overkill (how long does an isinstance really take?), but this is library code, and parse_indices gets call called a lot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No not overkill, you made a very good point about the duplication - but I should effectively revert the original suggestion to keep the conditionals simple given that
elif, whilst getting rid of the secondisinstancecall. Will do that now if you're happy...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1