Skip to content

DEP: Deprecate inexact matches for mode, searchside #16056

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

Merged
merged 5 commits into from
Jun 8, 2020

Conversation

anirudh2290
Copy link
Member

Depends on #16009

Please see the discussion here : #16007 (comment)

@anirudh2290 anirudh2290 force-pushed the dep_inexact_matches branch from 0d292b3 to 9b5fb65 Compare April 24, 2020 02:48
@eric-wieser eric-wieser changed the title DEP: Deprecate exact matches for mode, order, searchside DEP: Deprecate inexact matches for mode, order, searchside Apr 24, 2020
@eric-wieser
Copy link
Member

Think you had a typo in the PR title, corrected it.

@eric-wieser
Copy link
Member

Needs a rebase to clear out the first two commits

@eric-wieser eric-wieser marked this pull request as ready for review April 27, 2020 18:31
@anirudh2290 anirudh2290 force-pushed the dep_inexact_matches branch from 0a23278 to 92a4060 Compare April 29, 2020 02:35
@anirudh2290
Copy link
Member Author

  1. I deprecated byteorder inexact and case insensitive matches. For byteorder, I see that the test used 'big', 'little' etc. I changed it to 'B', 'L' etc. and deprecated the other non symbolic usages ('b' or 'big' or 'biggish').
  2. Removed the doc in certain places which have deprecated usage and added a deprecation note.
  3. Modified the tests and code which have deprecated usage.

.. deprecated:: 1.19.0

Case insensitive and inexact matches are deprecated for new_order.
For example, any of 'big', 'biggish' or 'b' in place of 'B' are
Copy link
Member

Choose a reason for hiding this comment

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

big and little are consistent with sys.byteorder, so I don't think we want to deprecate them.

Copy link
Member Author

Choose a reason for hiding this comment

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

I did this since some of the numpy documentation uses 'B' 'L' etc. for byte order : https://numpy.org/doc/stable/reference/generated/numpy.ndarray.newbyteorder.html?highlight=numpy%20ndarray%20newbyteorder#numpy.ndarray.newbyteorder and I felt deprecating such usage may have more impact on our existing users than in other deprecations. What do you suggest here ? Should we skip byteorder deprecation or just deprecate non exact matches of 'big', 'little' etc.

@anirudh2290 anirudh2290 force-pushed the dep_inexact_matches branch from a2c7da9 to e71e704 Compare April 29, 2020 23:24
@seberg
Copy link
Member

seberg commented May 4, 2020

Ah, I had missed the sortkind proposal. I am not sure I like it. First of all, we currently have quicksort in the documentation so I think that is good enough reason that we should keep it supported.
The question is whether we want to change the documentation to prefer the shorthand quick, but it seems to me that "quick" may be misleading, since it may be the slower sorter while "quicksort" is the name of the algorithm (for those who know it). (if it was algoirthm= and not kind= I would maybe think differently)

In other words, my first gut feeling would be to leave it as the current docs do it. I am happy to keep supporting quick and/or q as well though.

@seberg seberg added 07 - Deprecation 62 - Python API Changes or additions to the Python API. Mailing list should usually be notified. component: numpy._core labels May 4, 2020
@anirudh2290
Copy link
Member Author

The question is whether we want to change the documentation to prefer the shorthand quick, but it seems to me that "quick" may be misleading, since it may be the slower sorter while "quicksort" is the name of the algorithm (for those who know it). (if it was algoirthm= and not kind= I would maybe think differently)

I agree 'quick' vs 'quicksort' can cause some confusion. I think I will revert my changes for sortkind and byteorder for now and just keep the deprecation limited to mode, order and searchside for now.

@anirudh2290 anirudh2290 force-pushed the dep_inexact_matches branch from 02aea73 to cbdc377 Compare May 6, 2020 20:51
@seberg
Copy link
Member

seberg commented May 19, 2020

@anirudh2290 now that 1.19 is branched may be a good time to finish this. Just scrolled through, seems at least the "quick" documentation changes are still there.

@anirudh2290
Copy link
Member Author

Just scrolled through, seems at least the "quick" documentation changes are still there.

Thanks @seberg for pointing this out, for some reason I thought this was done. will check again .

@anirudh2290 anirudh2290 force-pushed the dep_inexact_matches branch from 60d8426 to ecd52ae Compare May 19, 2020 20:48
@anirudh2290 anirudh2290 reopened this May 26, 2020
@seberg seberg self-requested a review May 26, 2020 21:10
@anirudh2290 anirudh2290 force-pushed the dep_inexact_matches branch 2 times, most recently from fe9d21a to 652cee7 Compare May 29, 2020 02:19
@anirudh2290 anirudh2290 reopened this May 29, 2020
@anirudh2290 anirudh2290 closed this Jun 3, 2020
@anirudh2290 anirudh2290 reopened this Jun 3, 2020
@anirudh2290 anirudh2290 force-pushed the dep_inexact_matches branch from 652cee7 to d871057 Compare June 3, 2020 01:06
Copy link
Member

@seberg seberg left a comment

Choose a reason for hiding this comment

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

Thanks Anirudh, looks very nice. The only thing that makes me hesitate a bit is that I just checked scipy, and there are a few files that seem to use order='c' (it seems mainly two files, one file has order="f"), which might indicate that lower cases are used occasionally.

We may get away with it though, but could maybe fixup scipy before in either case? But if anyone has doubts, please say so, because otherwise one of these days I may decide to merge it :).

@mattip
Copy link
Member

mattip commented Jun 4, 2020

The mailing list discussion did not generate much feedback, but I am concerned that this may bite downstream packages and users. Are we really sure that pandas/astropy/Dask have no problem with this?

@seberg
Copy link
Member

seberg commented Jun 4, 2020

Well, I checked those large packages for the ones that would be likely order="f" (with lower case) and side="l" as a shorthand, and only found 3 files in SciPy.

I would be happy with keeping the lower case, if we expect those 3 usage/files in SciPy indicate that it is common enough that we do not want to create churn. That I feel would be the main point, unnecessary churn opposed to the advantage of unified style.

@mattip
Copy link
Member

mattip commented Jun 4, 2020

I would be happy with keeping the lower case, if we expect those 3 usage/files in SciPy indicate that it is common enough that we do not want to create churn.

That would be good

@anirudh2290 anirudh2290 force-pushed the dep_inexact_matches branch 2 times, most recently from cdfca00 to 8b97344 Compare June 5, 2020 18:17
@anirudh2290
Copy link
Member Author

Thanks @seberg and @mattip . I have addressed the review comments and removed order from deprecation to avoid churn. Thanks for checking the downstream projects @seberg ! I also ran the tests for pandas, dask, scipy and didnt find anything except the usage that you mentioned.

anirudh2290 and others added 4 commits June 5, 2020 18:25
Co-authored-by: Eric Wieser <wieser.eric@gmail.com>
Co-authored-by: Eric Wieser <wieser.eric@gmail.com>
Co-authored-by: Eric Wieser <wieser.eric@gmail.com>
@seberg seberg changed the title DEP: Deprecate inexact matches for mode, order, searchside DEP: Deprecate inexact matches for mode, searchside Jun 5, 2020
@anirudh2290 anirudh2290 force-pushed the dep_inexact_matches branch from 8b97344 to e47680f Compare June 5, 2020 19:40
@seberg seberg self-requested a review June 5, 2020 19:43
@seberg
Copy link
Member

seberg commented Jun 8, 2020

Thanks Anirudh! Let me put this in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
07 - Deprecation 62 - Python API Changes or additions to the Python API. Mailing list should usually be notified. component: numpy._core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants