8000 API: make numpy.lib._arraysetops.intersect1d work on multiple arrays by skuschel · Pull Request #25688 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

API: make numpy.lib._arraysetops.intersect1d work on multiple arrays #25688

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

API: make numpy.lib._arraysetops.intersect1d work on multiple arrays #25688

wants to merge 2 commits into from

Conversation

skuschel
Copy link

Intersect1d can be used with multiple arrays now and also returns the indices of all arrays when using return_indices=True.

I need this for my own work, so I figured I might as well share the code with the community. Thats why I didnt check on the mailing list first.

@skuschel skuschel force-pushed the intersect1d_multiplearrays branch from d58b386 to 751d238 Compare January 25, 2024 15:53
@skuschel
ENH: make numpy.lib._arraysetops.intersect1d work on multiple arrays
e2c956e 
Intersect1d can be used with multiple arrays now and also returns the
indices of all arrays when using `return_indices=True`.
@skuschel skuschel force-pushed the intersect1d_multiplearrays branch from 751d238 to e2c956e Compare January 25, 2024 15:58
@skuschel
Copy link
Author

I dont think the failing test is on me. Can someone restart this please or tell me what needs to change?

@ngoldbaum ngoldbaum changed the title ENH: make numpy.lib._arraysetops.intersect1d work on multiple arrays API: make numpy.lib._arraysetops.intersect1d work on multiple arrays Jan 25, 2024
@ngoldbaum
Copy link
Member

The doctest failure is real:

File "venv/lib/python3.11/site-packages/numpy/__init__.py", line ?, in intersect1d
Failed example:
    intersect1d(*ars, return_indices=True)
Exception raised:
    Traceback (most recent call last):
      File "/home/circleci/.pyenv/versions/3.11.4/lib/python3.11/doctest.py", line 1351, in __run
        exec(compile(example.source, filename, "single",
      File "<doctest intersect1d[2]>", line 1, in <module>
        intersect1d(*ars, return_indices=True)
        ^^^^^^^^^^^
    NameError: name 'intersect1d' is not defined

You can run the doctests locally with spin python tools/refguide_check.py --doctests.

The mypy failure is unrelated, I restarted that one.

By the way, this isn't mergeable without:

  • Some tests
  • API changes need more justification. The NumPy API is already quite complicated so we don't add things just because a contributor needed it for their work, they also have to justify why it's generally useful. You should ping the mailing list about this to get feedback from a broad cross-section of the community that doesn't track github every day.
  • API changes need a release note.

@ngoldbaum ngoldbaum added 56 - Needs Release Note. Needs an entry in doc/release/upcoming_changes 30 - API and removed 01 - Enhancement labels Jan 25, 2024
mhvk
mhvk reviewed Feb 2, 2024
View reviewed changes
Copy link
Contributor
@mhvk mhvk left a comment

Choose a reason for hiding this comment

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

As I also wrote on the mailing list, to me this looks like a nice generalization. It is probably good to check for other replies first, but if this goes forward, it indeed needs tests and a release note (since there is a small API change, in that assume_unique and return_indices now have to be keyword arguments).

@goruha goruha mentioned this pull request Mar 23, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mhvk mhvk mhvk left review comments

Assignees
No one assigned
Labels
30 - API 56 - Needs Release Note. Needs an entry in doc/release/upcoming_changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@skuschel @ngoldbaum @mhvk
0