Add Variable.__index__ #3689
Merged
Add Variable.__index__ #3689
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
MridulS
reviewed
Apr 24, 2025
There was a problem hiding this comment.
Pull Request Overview
This PR introduces Variable.index to enable type‐checked integer conversions, while updating related helper functions and tests to enforce stricter conditions on variables.
- Added new tests in variable_scalar_test.py and variable_creation_test.py to cover correct and incorrect use cases of index.
- Updated type constraints and helper functions in _binding.py and cpp_classes.pyi to support and bind the new index method.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/variable_scalar_test.py | Added tests to verify proper behavior of Variable.index. |
| tests/variable_creation_test.py | Updated tests to ensure variables used as indices are validated. |
| src/scipp/core/cpp_classes.pyi | Added index method declaration to the Variable class. |
| src/scipp/_binding.py | Introduced _index_dunder and updated conversion helper functions to bind index. |
MridulS
approved these changes
Apr 24, 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.
Fixes #3683
This is a bit odd because of how we implement
__getitem__. Normally, the arguments for a slice in__getitem__must satisfySupportsIndex, meaning that they must have an__index__method that returnsint. Unlike__int__,__index__is not supposed to convert floats to ints. E.g.,float.__index__does not exist andnp.ndarray.__index__raises an error if the dtype is not an integer.This PR implements
Variable.__index__similarly and rejects non-integer inputs. However, at runtime, we do allow floating point variables as indices in label-based indexing. (See the newtest_can_use_variable_as_index.) Fortunately, our code does not actually callVariable.__index__when constructing slices. So everything continues to work as before. But this leads to a disconnect. I think this is the best solution possible because our non-standard use of__getitem__will always lead to conflicts.