Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

gh-125420: implementSequence.__contains__ API onmemoryview 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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

@picnixz
Copy link
Member

@picnixzpicnixz commentedOct 14, 2024
edited by bedevere-appbot
Loading

@picnixzpicnixz changed the titlegh-125420: implement__contains__ tomemoryview objectsgh-125420: add__contains__ tomemoryview objectsOct 14, 2024
@picnixzpicnixz changed the titlegh-125420: add__contains__ tomemoryview objectsgh-125420: implementSequence.__contains__ API onmemoryview objectsOct 14, 2024
@picnixz
Copy link
MemberAuthor

Converting into a draft to decide whether the pure iterator approach is actually efficient enough or if iteration using the underlying multi-dimensional structure would be preferred (without spawning an iterator).

@picnixzpicnixz marked this pull request as draftOctober 14, 2024 13:03
@picnixz
Copy link
MemberAuthor

picnixz commentedOct 14, 2024
edited
Loading

For now, let's just keep this implementation. It's probably faster than the generic implementation since there are less code paths but it could definitely be faster by iterating directly over the buffer (and we don't seem to support iterating over multi-dimensional buffers yet).

@picnixzpicnixz marked this pull request as ready for reviewOctober 14, 2024 13:38
staticint
memory_contains(PyObject*self,PyObject*value)
{
PyObject*iter=PyObject_GetIter(self);
Copy link
Contributor

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 intensor-like-object.

E.g. I don't thinknumpy 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.

Copy link
Contributor

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 formemoryview isnumpy.array.
Although it is still very primitive in comparison, but it is possible this will change with time.

Copy link
MemberAuthor

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.

Copy link
Contributor

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.

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.

Note that this pattern is the pattern used by list objects.

What I am suggesting is thatmemoryview should be modelled aftertensor-like objects and not CPython sequences such aslist ortuple. It is slightly different breed, IMO.

Copy link
MemberAuthor

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).

Copy link
MemberAuthor

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...

Copy link
MemberAuthor

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.

Copy link
Contributor

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...

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 ofmemoryview and there is a reasonable chance that I will make some attempts in reasonably near future. I have been thinking about it approximately sincehttps://discuss.python.org/t/memoryview-multi-dimensional-slicing-support/52776.

And I personally don't mind updating what I wrote later.

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.

Copy link
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

past might not be a very good indicator for the future in this case.

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 thatin 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.

Copy link
Contributor

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 ofm-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. :)

@picnixzpicnixz added the staleStale PR or inactive for long period of time. labelJan 23, 2025
@picnixz
Copy link
MemberAuthor

Since we discussed about adding this oneif needs arise, I will just close them and delete my branch for now (I have too many local branches and I don't want to keep stale PRs). We'll be able to reuse it later if needed.

@picnixzpicnixz deleted the fix/memoryview-sequence-api-contains-125420 branchJanuary 23, 2025 14:25
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@JelleZijlstraJelleZijlstraAwaiting requested review from JelleZijlstra

1 more reviewer

@dg-pbdg-pbdg-pb left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

awaiting reviewstaleStale PR or inactive for long period of time.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@picnixz@dg-pb

[8]ページ先頭

©2009-2025 Movatter.jp