Skip to content

DEP: Deprecate inexact matches for mode, searchside#16056

Merged
seberg merged 5 commits intonumpy:masterfrom
anirudh2290:dep_inexact_matches
Jun 8, 2020
Merged

DEP: Deprecate inexact matches for mode, searchside#16056
seberg merged 5 commits intonumpy:masterfrom
anirudh2290:dep_inexact_matches

Conversation

@anirudh2290
Copy link
Copy Markdown
Member

@anirudh2290 anirudh2290 commented Apr 23, 2020

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 reviewed Apr 24, 2020
View reviewed changes
@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
Copy Markdown
Member

eric-wieser commented Apr 24, 2020

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

seberg
seberg reviewed Apr 24, 2020
View reviewed changes
@eric-wieser
Copy link
Copy Markdown
Member

eric-wieser commented Apr 27, 2020

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
eric-wieser
eric-wieser reviewed Apr 27, 2020
View reviewed changes
@anirudh2290 anirudh2290 force-pushed the dep_inexact_matches branch from 0a23278 to 92a4060 Compare April 29, 2020 02:35
@anirudh2290
Copy link
Copy Markdown
Member Author

anirudh2290 commented Apr 29, 2020

  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.

eric-wieser
eric-wieser reviewed Apr 29, 2020
View reviewed changes
eric-wieser
eric-wieser reviewed Apr 29, 2020
View reviewed changes
numpy/core/_add_newdocs.py Outdated
Copy link
Copy Markdown
Member

@eric-wieser eric-wieser Apr 29, 2020

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
Copy Markdown
Member Author

@anirudh2290 anirudh2290 Apr 29, 2020

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
Copy Markdown
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
Copy Markdown
Member Author

anirudh2290 commented May 4, 2020

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

eric-wieser
eric-wieser reviewed May 19, 2020
View reviewed changes
eric-wieser
eric-wieser reviewed May 19, 2020
View reviewed changes
@anirudh2290
Copy link
Copy Markdown
Member Author

anirudh2290 commented May 19, 2020

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 .

eric-wieser
eric-wieser reviewed May 19, 2020
View reviewed changes
eric-wieser
eric-wieser reviewed May 19, 2020
View reviewed changes
eric-wieser
eric-wieser reviewed May 19, 2020
View reviewed changes
eric-wieser
eric-wieser reviewed May 19, 2020
View reviewed changes
eric-wieser
eric-wieser reviewed May 19, 2020
View reviewed changes
@anirudh2290 anirudh2290 force-pushed the dep_inexact_matches branch from 60d8426 to ecd52ae Compare May 19, 2020 20:48
eric-wieser
eric-wieser reviewed May 19, 2020
View reviewed changes
@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
seberg
seberg approved these changes Jun 3, 2020
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member Author

anirudh2290 commented Jun 5, 2020

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
@anirudh2290 @eric-wieser
MAINT: Refactor to make conversion_util functions clearer
e918523 
Co-authored-by: Eric Wieser <wieser.eric@gmail.com>
@anirudh2290
BUG: Dont mask DeprecationWarning as ValueError
60f23b3
@anirudh2290 @eric-wieser
DOC: Add keyword inputs to byteorder doc
e75a481 
Co-authored-by: Eric Wieser <wieser.eric@gmail.com>
@anirudh2290 @eric-wieser
MAINT: Doc string for byteorder and deprecation warning message changes
5b5669f 
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
seberg
seberg reviewed Jun 5, 2020
View reviewed changes
@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
Copy Markdown
Member

seberg commented Jun 8, 2020

Thanks Anirudh! Let me put this in.

@seberg seberg merged commit 1d799fd into numpy:master Jun 8, 2020
This was referenced Oct 7, 2020
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@eric-wieser eric-wieser eric-wieser left review comments

@mattip mattip mattip approved these changes

@seberg seberg Awaiting requested review from seberg

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@anirudh2290 @eric-wieser @seberg @mattip