ENH: Improve np.linalg.det performance #28649
Conversation
There was a problem hiding this comment.
So far asv isn't showing much perf improvement on this branch on x86_64 Linux (i9-13900K):
asv continuous -E virtualenv -e -b "time_det.*" main linalg_refactor
BENCHMARKS NOT SIGNIFICANTLY CHANGED.
It may be because you're only making the first of the series of proposed changes.
Regardless of the performance changes, I suppose this is a reduction in lines of code, so maybe "ok" on its own anyway.
numpy/linalg/_linalg.py
Outdated
| @@ -152,8 +152,8 @@ def _commonType(*arrays): | ||
| for a in arrays: | ||
| type_ = a.dtype.type | ||
| if issubclass(type_, inexact): | ||
| if isComplexType(type_): | ||
| is_complex = True | ||
| is_complex = is_complex or isComplexType(type_) |
There was a problem hiding this comment.
Is this really worth doing? It takes my brain longer to process and shouldn't matter much performance-wise?
There was a problem hiding this comment.
I'm also not sure this routine is worth it, but if one goes for it, I'd start with something like
types = set(a.dtype.type for a in arrays)
which will generally reduce the number already, and then do
is_complex = any(isComplexType(type_) for type_ in types)
and something along similar lines for result_type (but with a check whether one can really not simply use the built-in np.result_type - not obvious to me).
But I'd do it in a separate PR.
There was a problem hiding this comment.
Interesting idea! It might not work out because the common use case is with len(arrays) just 1 or 2 and the set overhead is too large. I will remove this change here and test out in a new PR.
numpy/linalg/_linalg.py
Outdated
| @@ -152,8 +152,8 @@ def _commonType(*arrays): | ||
| for a in arrays: | ||
| type_ = a.dtype.type | ||
| if issubclass(type_, inexact): | ||
| if isComplexType(type_): | ||
| is_complex = True | ||
| is_complex = is_complex or isComplexType(type_) |
There was a problem hiding this comment.
I'm also not sure this routine is worth it, but if one goes for it, I'd start with something like
types = set(a.dtype.type for a in arrays)
which will generally reduce the number already, and then do
is_complex = any(isComplexType(type_) for type_ in types)
and something along similar lines for result_type (but with a check whether one can really not simply use the built-in np.result_type - not obvious to me).
But I'd do it in a separate PR.
| @@ -1320,11 +1317,6 @@ def eigvalsh(a, UPLO='L'): | ||
| w = gufunc(a, signature=signature) | ||
| return w.astype(_realType(result_t), copy=False) | ||
|
|
||
| def _convertarray(a): |
There was a problem hiding this comment.
Nice catch that this is not actually used!
numpy/linalg/_linalg.py
Outdated
| @@ -197,6 +197,9 @@ def _assert_stacked_2d(*arrays): | ||
|
|
||
| def _assert_stacked_square(*arrays): | ||
| for a in arrays: | ||
| if a.ndim < 2: |
There was a problem hiding this comment.
I like the combination. If one really wants to get out the most, it could be
try:
m, n = a.shape[-2:]
except ValueError:
riase LinAlgError(f"{a.ndim}-dimensional...") from None
if m != n:
...
Using that these days try/except has no cost if no exception is raised.
There was a problem hiding this comment.
Sorry, pushed this after submitting the review - above is the most relevant comment! (and not very relevant at that!)
There was a problem hiding this comment.
The _assert_stacked_square is about 10% faster using your suggestion, I updated the PR with this.
|
Thanks for reviewing! Next PR is #28686 |
…8649) * ENH: Improve np.linalg.det performance * Update numpy/linalg/_linalg.py * revert change to complex detection * use suggestion * whitespace * add more small array benchmarks * trigger build
We can improve performance of
np.linalg.detfor small arrays by up to 40% with 3 changes:_assert_stacked_2dcheck into_assert_stacked_square_commonTypeby using a cacher.astype(...)for scalar arguments making a copy of the data (and internally converting to an array and back)In this PR we perform the first step.