-
-
Notifications
You must be signed in to change notification settings - Fork 33.2k
gh-125420: implement Sequence.__contains__
API on memoryview
objects
#125441
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
Closed
picnixz
wants to merge
7
commits into
python:main
from
picnixz:fix/memoryview-sequence-api-contains-125420
Closed
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
5a93da1
add `__contains__` to memoryview objects
picnixz f90a3e1
blurb
picnixz 397bfe9
add fast path
picnixz e1f82a7
Merge branch 'main' into fix/memoryview-sequence-api-contains-125420
picnixz 50d7edd
Merge remote-tracking branch 'upstream/main' into fix/memoryview-sequ…
picnixz 2ec4368
Merge branch 'main' into fix/memoryview/contains-125420
picnixz ca91e9d
Merge branch 'main' into fix/memoryview/contains-125420
picnixz 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
2 changes: 2 additions & 0 deletions
2
Misc/NEWS.d/next/Core_and_Builtins/2024-10-14-12-17-00.gh-issue-125420.hpMz9A.rst
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
Implement :meth:`~object.__contains__` for :class:`memoryview` objects. | ||
Patch by Bénédikt Tran. |
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
Oops, something went wrong.
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 it might be good to follow the pattern of already implemented methods and have it a bit faster.
Creating iterators when they aren't needed might not be the best option in
tensor-like-object
.E.g. I don't think
numpy
doesobj in array.flat
fora in array
.I think it is much easier to do it from the beginning compared to all convincing that will be required to change this 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 the best reference for such decisions for
memoryview
isnumpy.array
.Although it is still very primitive in comparison, but it is possible this will change with time.
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.
As I said on the issue, performances should be addressed in a follow-up PR. Note that this pattern is the pattern used by list objects.
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 don't think this is only about the performance, but also about the design.
Also, such follow-up PR would pretty much replace the whole implementation of this PR.
What I am suggesting is that
memoryview
should be modelled aftertensor-like
objects and not CPython sequences such aslist
ortuple
. It is slightly different breed, IMO.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.
For now, memoryviews are only 1-dimensional =/ we don't have multi-dimensional slicing. And it's been like this for ages. Tensors are generally used for generalizing vectors and matrices, but for now, we do not support them at all.
I can try to use lower-level calls, but this would overcomplicate the implementation itself. My original idea was to inline most of the calls to avoid materializing an iterator and advance item by item manually. But I expected the implementation to be harder to maintain. As Jelle said,
in
already worked because it delegated toPySequence_Contains
. What we needed in this PR was to have an explicitSequence.__contains__
(same forSequence.__reversed__
of the other PR).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, because the "natural evolution" hasn't changed for the past 10 years...
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.
And I personally don't mind updating what I wrote 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.
It did not evolve much over past 10 years, yes. From my POV, past might not be a very good indicator for the future in this case. I think there are some overlooked opportunities here.
I am personally interested in multi-dimensional slicing of
memoryview
and there is a reasonable chance that I will make some attempts in reasonably near future. I have been thinking about it approximately since https://discuss.python.org/t/memoryview-multi-dimensional-slicing-support/52776.This doesn't make much difference, whether it is you or someone else, the biggest cost here is new PR, review process, time delays, etc. Implementation itself would be a minor issue 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.
Unfortunately, it is for most core devs. I will not change my stance on this matter for now, because my aim was to port the implicit dispatch (remember that
in
already works because it dispatches toSequence_Contains
) to an explicit one. If you want an implementation using the index-based approach, feel free to open an alternate PR.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.
It is not as big of a deal as it might seem that I am trying to make. :) Given it happens, this will most likely be changed as part of
m-dim-slicing
by whoever will be doing that.But I am still finding trouble to see why not implement it more correctly from the beginning given such low cost. :)