Skip to content
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

MAINT: extend delete single value optimization #16895

Merged
merged 4 commits into from
Feb 27, 2022

Conversation

DevinShanahan
Copy link
Contributor

Allow arrays of shape (1,) for np.delete's obj parameter to utilize the optimization for a single value. See #16685.

In [2]: arr = np.arange(3000000).reshape(1000, 3000)      
                      
In [3]: %timeit np.delete(arr, obj=0, axis=1)                                   
6.91 ms ± 352 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)

In [4]: %timeit np.delete(arr, obj=[0], axis=1)                                 
6.72 ms ± 245 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)

In [5]: zero = np.array([0])                                                    

In [6]: %timeit np.delete(arr, obj=zero, axis=1)                                
6.77 ms ± 242 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)

Allow arrays of shape (1,) for delete's obj parameter to utilize the 
optimization for a single value. See numpy#16685.
@charris charris changed the title ENH: extend delete single value optimization MAINT: extend delete single value optimization Jul 18, 2020
Co-authored-by: Eric Wieser <wieser.eric@gmail.com>
mattip
mattip previously approved these changes Aug 13, 2020
Copy link
Member

@mattip mattip left a comment

Choose a reason for hiding this comment

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

Closes #16685

obj = obj.astype(intp)

elif obj.shape == (1,):
obj = obj.item()
Copy link
Member

Choose a reason for hiding this comment

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

item() makes me worry here...

Copy link
Member

Choose a reason for hiding this comment

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

Does this patch change the behavior of np.delete(np.arange(10), np.array([1], dtype=object))?

Copy link
Member

Choose a reason for hiding this comment

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

I think obj = obj.reshape(()) would avoid that shortcoming - please add a test!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could your clarify the behavior you are concerned about for np.delete(np.arange(10), np.array([1], dtype=object)). I will change it to obj = obj.reshape(()) either way but the behavior appears the same to me. What would you like to see in the test?

Also, is there anything else you would like to see tested?

@mattip mattip dismissed their stale review August 17, 2020 14:10

More tests needed

Base automatically changed from master to main March 4, 2021 02:05
@InessaPawson
Copy link
Member

@DevinShanahan Do you think you could finish this PR? It would be nice to fix #16685.

@DevinShanahan
Copy link
Contributor Author

@InessaPawson Yes, I will try to complete this as soon as possible.

@DevinShanahan
Copy link
Contributor Author

@InessaPawson I've re-requested a review, anything else you need from me on this at this time?

@InessaPawson
Copy link
Member

@DevinShanahan Thank you for updating the PR! We will review it shortly.

@mattip mattip merged commit 3b03c2a into numpy:main Feb 27, 2022
@mattip
Copy link
Member

mattip commented Feb 27, 2022

Thanks @DevinShanahan

@InessaPawson
Copy link
Member

Hi-five on merging your first pull request to NumPy, @DevinShanahan! We hope you stick around! Your choices aren’t limited to programming – you can review pull requests, help us stay on top of new and old issues, develop educational material, work on our website, add or improve graphic design, create marketing materials, translate website content, write grant proposals, and help with other fundraising initiatives. For more info, check out: https://numpy.org/contribute
Also, consider joining our mailing list. This is a great way to connect with other cool people in our community and be part of important conversations that affect the development of NumPy: https://mail.python.org/mailman/listinfo/numpy-discussion

@seberg
Copy link
Member

seberg commented Jun 23, 2022

This PR broke bolean array inputs, see gh-21840 (at least I am practically certain). Unfortunately, we need to either carefully check all the paths the "integer array" paths (which seem to accept booleans arrays now) or simply revert it and give it another shot some other time.

seberg added a commit to seberg/numpy that referenced this pull request Jun 27, 2022
Non integer array-likes were not correctly rejected when a new
fast-path was added to `np.delete` in numpygh-16895.
This includes the _explicitly_ added `dtype=object` which should
not be allowed since it is not allowed in normal indexing either.

Closes numpygh-21840
rossbar pushed a commit that referenced this pull request Jun 28, 2022
Non integer array-likes were not correctly rejected when a new
fast-path was added to `np.delete` in gh-16895.
This includes the _explicitly_ added `dtype=object` which should
not be allowed since it is not allowed in normal indexing either.

Closes gh-21840
charris pushed a commit to charris/numpy that referenced this pull request Jun 28, 2022
Non integer array-likes were not correctly rejected when a new
fast-path was added to `np.delete` in numpygh-16895.
This includes the _explicitly_ added `dtype=object` which should
not be allowed since it is not allowed in normal indexing either.

Closes numpygh-21840
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants