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: Improve performance of common_type and use result_type internally #24668

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

eendebakpt
Copy link
Contributor

@eendebakpt eendebakpt commented Sep 7, 2023

Cleanup implementation of np.common_type using np.result_type and improve performance.

i16=np.array(2, dtype=np.int16)
f32=np.array(2, dtype=np.float32)
np.common_type(i16, f32)

now returns float32 instead of float64.

@seberg
Copy link
Member

seberg commented Mar 6, 2024

Using result_type should be good, let's give this a shot, thanks!

@seberg
Copy link
Member

seberg commented Mar 6, 2024

Ohh, now I see that the reason I must have hesitated is the behavior change. Which may be fine, but would maybe need a shout-out.
How about:

dt = result_type(dtypes)
if not inexact:
   dt = result_type(np.float64, dt)
   if not inexact:
       raise

as logic, so that we can avoid that one change? I am OK to not worry about timedelta. Since float64 is the largest float necessary for any integer promotion, I think we can gamble on that being good.

(I hate this function, but not sure I want to change it...)

@eendebakpt
Copy link
Contributor Author

Ohh, now I see that the reason I must have hesitated is the behavior change. Which may be fine, but would maybe need a shout-out. How about:

dt = result_type(dtypes)
if not inexact:
   dt = result_type(np.float64, dt)
   if not inexact:
       raise

as logic, so that we can avoid that one change? I am OK to not worry about timedelta. Since float64 is the largest float necessary for any integer promotion, I think we can gamble on that being good.

(I hate this function, but not sure I want to change it...)

With your suggestion we still have the behaviour change right? The result of np.result_type(np.int16, np.float32) is dtype('float32'), which is inexact so the part result_type(np.float64, dt) is never reached.

I see now that the documentation of common_type explicitly states "If one of the inputs is an integer array, the minimum precision type that is returned is a 64-bit floating point dtype.". So if we allow the behaviour change, we should update the documentation and make a clear release note.

I think that if we want to enforce that a single integer input type enforces the output is 64-bit float (at minimum) we have no alternative than to check each argument for being integer. In that case we fall back to #24467 or some combination that I suspect will be slower than the current implementation.

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.

2 participants