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: Replace find_common_types #16398

Merged
merged 1 commit into from Jun 14, 2022
Merged

Conversation

eendebakpt
Copy link
Contributor

@eendebakpt eendebakpt commented Jun 13, 2022

What does this implement/fix?

The np.find_common_type method is deprecated within numpy. See numpy/numpy#21703 and numpy/numpy#21742.
This PR was triggered because np.find_common_type is one of performance bottlenecks inside the scipy.stats.rv_continuous.

This PR replaces the np.find_common_type with np.results_type or np.promote_type. These alternatives are both faster and maintained.

Additional information

Microbenchmark results
In [45]: runcell(1, '/home/eendebakpt/sympy/sympy/geometry/tests/untitled13.py')
float64
9.25 µs ± 672 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each)

In [46]: import numpy as np
    ...: from numpy.core.numerictypes import find_common_type
    ...: from numpy import promote_types , result_type
    ...: 
    ...: array_types = [np.dtype('float64'), np.dtype('float64')]
    ...: 
    ...: print(find_common_type(array_types, []))
    ...: %timeit find_common_type(array_types, [])
float64
8.62 µs ± 234 ns per loop (mean ± std. dev. of 7 runs, 100,000 loops each)

In [47]: print(promote_types(*array_types))
    ...: %timeit promote_types(*array_types)
float64
117 ns ± 2.63 ns per loop (mean ± std. dev. of 7 runs, 10,000,000 loops each)

@@ -155,7 +155,7 @@ def _nbool_correspond_all(u, v, w=None):
ntf = (u & not_v).sum()
ntt = (u & v).sum()
else:
dtype = np.find_common_type([int], [u.dtype, v.dtype])
dtype = np.result_type(int, u.dtype, v.dtype)
Copy link
Contributor

Choose a reason for hiding this comment

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

In theory, this can change the result in some cases, but in practice I can't think of a way to make it fail when the first part is [int]...
(in that case u.dtype == longdouble is honored. A change would happen there if the first part was a float/complex, where the longdouble would then be ignored.)

@eendebakpt eendebakpt marked this pull request as ready for review June 13, 2022 19:40
@rgommers rgommers added the maintenance Items related to regular maintenance tasks label Jun 13, 2022
@tupui tupui added this to the 1.10.0 milestone Jun 13, 2022
Copy link
Member

@perimosocordiae perimosocordiae left a comment

Choose a reason for hiding this comment

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

scipy.sparse changes look fine

Copy link
Member

@tupui tupui left a comment

Choose a reason for hiding this comment

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

LGTM, we have some approvals and CI is happy. In it goes. Thank you @eendebakpt et al.!

@tupui tupui merged commit 66b99ff into scipy:main Jun 14, 2022
@eendebakpt eendebakpt deleted the replace_find_common_type branch June 14, 2022 09:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Items related to regular maintenance tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants