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

Create cat regressor #3353

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open
1 change: 1 addition & 0 deletions docs/release-notes/3353.performance.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
* Speed up for a categorical regressor in {func}`~scanpy.pp.regress_out` {smaller}`S Dicks`
26 changes: 20 additions & 6 deletions src/scanpy/preprocessing/_simple.py
Original file line number Diff line number Diff line change
Expand Up @@ -628,6 +628,20 @@
DT = TypeVar("DT")


@njit
def _create_regressor_categorical(
X: np.ndarray, number_categories: int, filters: np.ndarray
) -> np.ndarray:
# create regressor matrix faster for categorical variables
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this comment mean?

regressors = np.zeros(X.shape, dtype=X.dtype)
XT = X.T
for category in range(number_categories):
mask = category == filters
for ix in numba.prange(XT.shape[0]):
regressors[mask, ix] = XT[ix][mask].mean()
flying-sheep marked this conversation as resolved.
Show resolved Hide resolved
return regressors

Check warning on line 642 in src/scanpy/preprocessing/_simple.py

View check run for this annotation

Codecov / codecov/patch

src/scanpy/preprocessing/_simple.py#L636-L642

Added lines #L636 - L642 were not covered by tests


@njit
def get_resid(
data: np.ndarray,
Expand Down Expand Up @@ -722,13 +736,13 @@
"we regress on the mean for each category."
)
logg.debug("... regressing on per-gene means within categories")
regressors = np.zeros(X.shape, dtype="float32")
# Create numpy array's from categorical variable
number_categories = np.int64(len(adata.obs[keys[0]].cat.categories))
filters = adata.obs[keys[0]].cat.codes.to_numpy()
number_categories = number_categories.astype(filters.dtype)
Comment on lines +740 to +742
Copy link
Member

@flying-sheep flying-sheep Nov 14, 2024

Choose a reason for hiding this comment

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

Either this or add a comment (to the code) explaining why it needs to be the other way.
Also if I do this, the test still passes, so …

Suggested change
number_categories = np.int64(len(adata.obs[keys[0]].cat.categories))
filters = adata.obs[keys[0]].cat.codes.to_numpy()
number_categories = number_categories.astype(filters.dtype)
number_categories = len(adata.obs[keys[0]].cat.categories)
filters = adata.obs[keys[0]].cat.codes.to_numpy()

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a comment. Other wise you have a dtype missmatch and crash of the kernel

Copy link
Contributor

Choose a reason for hiding this comment

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

Other wise you have a dtype missmatch and crash of the kernel

I would say that this is the important part for the comment!

Copy link
Member

@flying-sheep flying-sheep Nov 21, 2024

Choose a reason for hiding this comment

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

100%!

  • refactor your code until the “what” is obvious.
  • if the “why” isn’t obvious from understanding the “what”, add the missing parts as a comment

I see that you’re

  1. convert the cat codes into a numpy array
  2. creating a numpy scalar with the same dtype as filters, holding the number of categories

So you don’t need to comment that you do any of that.

I asked because I’m confused why a Python integer is converted to a numpy scalar: Usually APIs accept either and do the converting themselves. So I’d like to see a comment removing that confusion by explaining why you convert to a numpy scalar. (a crash is a great reason)


but I also see that _create_regressor_categorical has number_categories: int and then does range(number_categories), so I’m still very confused why numba crashes unless the dtypes match.

I can’t reproduce the crash. leaving the thing as a Python int just works for me.

Copy link
Member

Choose a reason for hiding this comment

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

Also the way to do this in one step is

Suggested change
number_categories = np.int64(len(adata.obs[keys[0]].cat.categories))
filters = adata.obs[keys[0]].cat.codes.to_numpy()
number_categories = number_categories.astype(filters.dtype)
filters = adata.obs[keys[0]].cat.codes.to_numpy()
number_categories = filters.dtype.type(len(adata.obs[keys[0]].cat.categories))


X = _to_dense(X, order="F") if issparse(X) else X
# TODO figure out if we should use a numba kernel for this
for category in adata.obs[keys[0]].cat.categories:
mask = (category == adata.obs[keys[0]]).values
for ix, x in enumerate(X.T):
regressors[mask, ix] = x[mask].mean()
regressors = _create_regressor_categorical(X, number_categories, filters)
variable_is_categorical = True
# regress on one or several ordinal variables
else:
Expand Down
Binary file added tests/_data/regress_test_small_cat.npy
Binary file not shown.
10 changes: 10 additions & 0 deletions tests/test_preprocessing.py
Original file line number Diff line number Diff line change
Expand Up @@ -337,6 +337,16 @@ def test_regress_out_reproducible():
np.testing.assert_allclose(adata.X, tester)


def test_regress_out_reproducible_category():
adata = sc.datasets.pbmc68k_reduced()
adata = adata.raw.to_adata()[:200, :200].copy()
sc.pp.regress_out(adata, keys=["bulk_labels"])
# This file was generated from the original implementation in version 1.10.3
# Now we compare new implementation with the old one
tester = np.load(DATA_PATH / "regress_test_small_cat.npy")
np.testing.assert_array_almost_equal(adata.X, tester)
flying-sheep marked this conversation as resolved.
Show resolved Hide resolved


def test_regress_out_constants_equivalent():
# Tests that constant values don't change results
# (since support for constant values is implemented by us)
Expand Down
Loading