ENH: allow int sequences as shape arguments in numpy.memmap #23729
Conversation
This file contains 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
The constructor for memmap objects initially required the shape argument to be a tuple, where non-tuple shape arguments would be made into a single member tuple. This behaviour was intended to allow an integer shape arguments. However this caused list or numpy.array shape arguments to raise errors within the constructor. Modifications made to the shape argument handling within the constructor now allow for integer sequences to be used for shape. A test was added and constructor docstring was updated to account for these changes.
The constructor for memmap objects initially required the shape argument to be a tuple, where non-tuple shape arguments would be made into a single member tuple. This behaviour was intended to allow an integer shape arguments. However this caused list or numpy.array shape arguments to raise errors within the constructor. Modifications made to the shape argument handling within the constructor now allow for integer sequences to be used for shape. A test was added and constructor docstring was updated to account for these changes.
seberg
reviewed
May 8, 2023
numpy/core/memmap.py
Outdated
| @@ -242,8 +242,10 @@ def __new__(subtype, filename, dtype=uint8, mode='r+', offset=0, | |||
| size = bytes // _dbytes | |||
| shape = (size,) | |||
| else: | |||
| if not isinstance(shape, tuple): | |||
| if not hasattr(shape, '__iter__'): | |||
There was a problem hiding this comment.
I would suggest using the same pattern as normalize_axis_tuple (I think we may have another pattern around). You are adding int support, so should have a .. versionchanged:: tag (and release note if you like).
(That will allow np.array(5) as a shape. Not that that should matter in practice, just think it may make sense to reuse a pattern we use in a few places.)
The tuple check is fine, but I suspect not worth it if it is already a tuple calling tuple is a no-op (except for subclasses maybe).
Type handling within np.memmap constructor now matches the type handling pattern found in normalize_axis_tuple. The test for type handling was tested with 4 cases. Within the documentation, the shape argument now has a version changed tag. A release note has been added for this improvement.
|
Just a clarification, the versionchanged tag should be in reference to version 1.25? |
seberg
approved these changes
Jun 1, 2023
seberg
reviewed
Jun 1, 2023
|
Thanks @JanukanS. |
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.
The constructor for memmap objects initially required the shape argument to be a tuple, where non-tuple shape arguments would be made into a single member tuple. This behaviour was intended to allow an integer shape arguments. However this caused list or numpy.array shape arguments to raise errors within the constructor. Modifications made to the shape argument handling within the constructor now allow for integer sequences to be used for shape. A test was added and constructor docstring was updated to account for these changes.