Repository navigation
BUG: Do not forward descending unless explicitly passed - #31458
Conversation
jorenham
changed the title
| extra_kw = {"descending": descending} | ||
| else: | ||
| extra_kw = {} | ||
| a.sort(axis=axis, kind=kind, order=order, stable=stable, **extra_kw) |
There was a problem hiding this comment.
Awesome, thanks @pllim. I would be tempted to just duplicating the return a.sort(...) (to avoid creating a dict just to unpack it immediately).
A test would be great, but I am also fine if not. (Needs a small dummy object -- I guess in test_multiarray although that file is annoying large.)
(We sometimes use np._NoValue, but I think in this case it should be fine to just not pass on if the user passes descending=False explicitly.)
Both are somewhat nits though, I admit :).
We probably should do the same change to argsort(). (It uses wrapfunc, but that is just because it doesn't work in-place mainly, I guess. We can do just the same and duplicate.)
There was a problem hiding this comment.
Thanks for the detailed feedback! I implemented the functional code changes, hopefully as you requested.
However, I noticed there are already a bunch of test_sort_descending tests in numpy/_core/tests/test_multiarray.py. I am not sure how to write another test that would trigger this error without introducing that class from astropy. 😬
There was a problem hiding this comment.
Thank you for fixing this @pllim! About the test, I think you could create a small dummy class (does not need to have any real functionality or mirror astropy) that implements argsort with the right (kw)args, then write a test that instantiates an object and checks there is no error when calling np.argsort(a, descending=False).
but also do not unpack kwargs so just duplicate the call.
|
For the |
|
I pushed a test, but someone should have a quick look (verified that it failed for both sort and argsort before). |
| a = asanyarray(a).copy(order="K") | ||
| a.sort(axis=axis, kind=kind, order=order, stable=stable, descending=descending) | ||
| # Sanitize for backward-compatibility | ||
| if descending: |
There was a problem hiding this comment.
In other code, we would set the argument to np._NoValue and pass on the argument if it is not that (see, e.g., argmax in the same file). I think that is better, because then if someone explicitly passes in descending=False, they get the correct error that the argument does not exist.
There was a problem hiding this comment.
Hmmmm, yeah, let's go with this. I somewhat thought that here the default may be unambiguous enough, but really... it is also for keepdims= probably and safer.
|
Thank you for adding the test here, @seberg ! Given I am unfamiliar with your testing setup, it is more elegant than what I could have come up with. Is there anything else you need from me? If not, I'll let you do your thing. And thanks for appreciating the branch name. |
This is just a bit more precise to not pass things on when explicitly passed as `descending=False`.
|
OK, changed to |
|
Hmm CI is failing. Is that related? |
seberg
changed the title
|
Thank you for cleaning up my PR and bringing it over the finish line! I appreciate the timely response. |
PR summary
My interpretation of #31345 (comment) from @seberg
First time committer introduction
👋 from Astropy to fix downstream failure (astropy/astropy#19760)
AI Disclosure
I hate AI and also too cheap to pay for one.