-
Notifications
You must be signed in to change notification settings - Fork 616
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
fix int for shortcut #3461
base: main
Are you sure you want to change the base?
fix int for shortcut #3461
Conversation
src/scanpy/preprocessing/_simple.py
Outdated
@@ -752,6 +752,9 @@ def regress_out( | |||
# if the regressors are not categorical and the matrix is not singular | |||
# use the shortcut numpy_regress_out | |||
if not variable_is_categorical and np.linalg.det(regressors.T @ regressors) != 0: | |||
if np.issubdtype(X.dtype, np.integer): | |||
target_dtype = np.float32 if X.dtype.itemsize <= 4 else np.float64 | |||
X = X.astype(target_dtype) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For dense you can also do the ordering here, since you're copying anyway
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3461 +/- ##
==========================================
- Coverage 75.44% 75.43% -0.01%
==========================================
Files 113 113
Lines 13250 13253 +3
==========================================
+ Hits 9997 9998 +1
- Misses 3253 3255 +2
|
@@ -754,7 +754,7 @@ def regress_out( | |||
if not variable_is_categorical and np.linalg.det(regressors.T @ regressors) != 0: | |||
if np.issubdtype(X.dtype, np.integer): | |||
target_dtype = np.float32 if X.dtype.itemsize <= 4 else np.float64 | |||
X = X.astype(target_dtype) | |||
X = X.astype(target_dtype, order="C") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But this could also apply to sparse? And we don't want the argument then, no? https://docs.scipy.org/doc/scipy/reference/generated/scipy.sparse.csr_matrix.astype.html nothing here about order
Fixes a bug where the regress_out short cut would error for integer dyte