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

PERF: Micro optimize find_common_type #21742

Closed

Conversation

eendebakpt
Copy link
Contributor

The find_common_type is a (small) performance bottleneck in one of the scipy algoritms. This PR optimizes the method:

A benchmark (extracted from what is called from scipy):

import numpy as np
from numpy.core.numerictypes import find_common_type

array_types = [np.dtype('float64'), np.dtype('float64')]
scalar_types = []

find_common_type(array_types, scalar_types)

Results of %timeit find_common_type(array_types, scalar_types).

  • Main: 8.37 µs ± 500 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)
  • PR: 1.09 µs ± 57.6 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)
  • PR (only first commit): 1.81 µs ± 313 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)

The optmizations are:

  • Cache cast of __test_types to dtype
  • Make elements of dtypelist unique. This could make other calls a bit slower, but the list(set(dtypelist)) is fast and it makes cases with duplicate dtypes faster
  • Fast path in find_common_type for empty arguments.

@seberg
Copy link
Member

seberg commented Jun 13, 2022

Sorry, but I am pretty firmly 👎 because this function should probably never be used and I would like to burn this function in a volcano, see also gh-21703. np.promote_types or np.result_typeshould be used instead.

EDIT: But, the code change itself looks simple and fine. So if anyone other core dev likes this independent of the fact that the function should be deprecated, just go ahead.

@mattip
Copy link
Member

mattip commented Jun 13, 2022

Can we suggest a fix to scipy to avoid this function that is also more performant?

@mattip
Copy link
Member

mattip commented Jun 13, 2022

Ahh, you already did :). Since the alternatives are more performant, I think we should close this and deprecate find_common_type

@eendebakpt
Copy link
Contributor Author

Ahh, you already did :). Since the alternatives are more performant, I think we should close this and deprecate find_common_type

Since there is a PR at scipy I am fine with closing this. The only reason to merge would be if (even when deprecating) the find_common_type would stick around a long time to support external pacakges.

@eendebakpt
Copy link
Contributor Author

Closing this PR. The issue has been solved in scipy.

@eendebakpt eendebakpt closed this Jun 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants