Skip to content

DEP: Deprecate np.find_common_type#22539

Merged
charris merged 11 commits intonumpy:mainfrom
seberg:deprecate-find-common-type
May 13, 2023
Merged

DEP: Deprecate np.find_common_type#22539
charris merged 11 commits intonumpy:mainfrom
seberg:deprecate-find-common-type

Conversation

@seberg
Copy link
Copy Markdown
Member

@seberg seberg commented Nov 7, 2022

The function uses the numeric scalar common dtype/promotion rules.
These are subtly different from the typical NumPy rules defined by
np.result_type.
Mainly, there is no good reason to have two subtly different rules
exposed and find_common_type is less reliable, slower, and not really
maintainable when it comes to NEP 50.

Closes #21703.


The function was used once within NumPy, and that is in the AxisConcatenator which implements np.r_ and np.c_. I replaced the occurrence there for np.result_type which mildly changes behavior in some odd cases like:

np.r_[np.arange(10, dtype=np.uint8), -1]  # int16 result rather than int64
np.r_[np.arange(10, dtype=np.int8), 255]  # int16, rather than int8

(cases where unsigned arrays are mixed with signed scalars (not 0-D arrays even), and cases which currently lead to overflows during casts.)

Closes gh-21703

Rather, use `result_type` instead.  There are some exceedingly small
theoretical changes, since `result_type` currently uses value-inspection
logic.
`find_common_type` did not, because it pre-dates the value inspection
logic.
(I.e. in theory, this switches it to value-based promotion, just to
partially undo that in NEP 50; although more changes there.)

The only place where it is fathomable to matter is if someone is using
`np.c_[uint8_arr, -1]` to append 255 to an unsigned integer array.
The function uses the numeric scalar common dtype/promotion rules.
These are subtly different from the typical NumPy rules defined by
`np.result_type`.
Mainly, there is no good reason to have two subtly different rules
exposed and `find_common_type` is less reliable, slower, and not really
maintainable when it comes to NEP 50.
Unfortunately, the notes for the `np.r_` change is rather long even
though it is not a change I expect to hit a whole lot of users...
@seberg seberg force-pushed the deprecate-find-common-type branch from c4e4bd7 to 3c15610 Compare November 7, 2022 14:13
Copy link
Copy Markdown
Contributor

@rossbar rossbar left a comment

Choose a reason for hiding this comment

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

I'm +1 for the deprecation, especially given the changes proposed in NEP50.

FWIW I grepped other scientific Python libraries and did notice a few instances in e.g. scikit-learn (3) and astropy (1), but they were all of the scalar_types=[] variety, so replacing with result_type should be straightforward.

@@ -0,0 +1,24 @@
``np.find_common_type`` is deprecated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is a lot of info for a release note but then again the situation is complicated. I'm +1 for leaving the info in the release notes instead of the docs proper since this is a deprecation and the complexity will be reduced in the future.

@seberg
Copy link
Copy Markdown
Member Author

seberg commented Nov 8, 2022

The pandas PR highlights two small changes I didn't notice:

  • find_common_type seems to return object when it doesn't know (which might require a try/except clause if you really want it.
  • find_common_type knows nothing about (e.g.) timedelta/datetime and if there is more than one dtype and a time, it just bails to object.

Seems unlikely many will run into this (or even prefer no error), but pandas seems to...

@seberg seberg force-pushed the deprecate-find-common-type branch from 43e239a to 1ad0166 Compare November 8, 2022 14:27
Copy link
Copy Markdown
Contributor

@rossbar rossbar left a comment

Choose a reason for hiding this comment

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

Thanks @seberg , this LGTM give or take the remaining typing re-exportation.

@charris any objections to getting this in for 1.24?

@rossbar
Copy link
Copy Markdown
Contributor

rossbar commented Nov 16, 2022

We discussed this at the triage meeting and decided to wait until after 1.24 is out to merge this.

@bsipocz
Copy link
Copy Markdown
Member

bsipocz commented Nov 16, 2022

This would close #21703, could someone link it in the OP or "Development"?

@charris
Copy link
Copy Markdown
Member

charris commented Feb 19, 2023

close/reopen

@charris charris closed this Feb 19, 2023
@charris charris reopened this Feb 19, 2023
@charris
Copy link
Copy Markdown
Member

charris commented Feb 19, 2023

@seberg Be good to finish this up,

@charris
Copy link
Copy Markdown
Member

charris commented Feb 19, 2023

I think the circleci failure will go away with a rebase.

@seberg seberg modified the milestone: 1.25.0 release May 8, 2023
@charris charris merged commit 7cc7254 into numpy:main May 13, 2023
@charris
Copy link
Copy Markdown
Member

charris commented May 13, 2023

Thanks Sebastian.

@seberg
Copy link
Copy Markdown
Member Author

seberg commented May 15, 2023

Thanks for fixing this up! I hope its not annoying to adapt to without gh-22568, although I guess chances are mainly/only pandas notices, so maybe that is OK if it is?

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.

DEP: Deprecate find_common_type which is broken e.g. for datetimes

6 participants