Skip to content

Speed up categorical regressor with numba #3353

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

Open
wants to merge 32 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
086f70d
add function and test
Intron7 Nov 11, 2024
37244a9
add test
Intron7 Nov 11, 2024
b4ecb0a
add test for regressor
Intron7 Nov 11, 2024
36858d9
add release note
Intron7 Nov 11, 2024
be1bccc
Merge branch 'main' into create_cat_regressor
Intron7 Nov 11, 2024
a1a59ae
update typing
Intron7 Nov 11, 2024
7b41bc8
update test
Intron7 Nov 11, 2024
119a142
update test
Intron7 Nov 12, 2024
d77fa9c
update dtype
Intron7 Nov 12, 2024
236e356
rename cats
Intron7 Nov 12, 2024
bb9cde4
Update tests/test_preprocessing.py
Intron7 Nov 12, 2024
bbb5035
Update tests/test_preprocessing.py
Intron7 Nov 12, 2024
2a92193
Update tests/test_preprocessing.py
Intron7 Nov 12, 2024
c7b78c0
Update tests/test_preprocessing.py
ilan-gold Nov 12, 2024
b001c0e
remove test
Intron7 Nov 13, 2024
c3ce03e
update kernel
Intron7 Nov 13, 2024
c50226a
remove test
Intron7 Nov 13, 2024
c6665f4
make test together
Intron7 Nov 21, 2024
858e247
cleanup
Intron7 Nov 21, 2024
2421bd5
add comment
Intron7 Nov 21, 2024
2e16c45
Merge branch 'main' into create_cat_regressor
Intron7 Dec 16, 2024
1b7d7e1
Merge branch 'main' into create_cat_regressor
Intron7 Jan 23, 2025
3b7fe6e
Merge branch 'main' into create_cat_regressor
Intron7 Feb 10, 2025
726a625
update doc strings and clean up names
Intron7 Feb 10, 2025
104a0f3
Update src/scanpy/preprocessing/_simple.py
Intron7 Feb 11, 2025
f9b13be
update dtypes
Intron7 Feb 11, 2025
6eafd04
update atol for test
Intron7 Feb 11, 2025
1dae8f4
remove int fix
Intron7 Feb 11, 2025
39ad1c0
Update docs/release-notes/3353.performance.md
Intron7 Apr 14, 2025
4f3db86
Merge branch 'main' into create_cat_regressor
Intron7 Apr 14, 2025
2d578c8
Update src/scanpy/preprocessing/_simple.py
Intron7 Apr 14, 2025
eedb314
Fix sparse check
flying-sheep Apr 14, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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`
27 changes: 21 additions & 6 deletions src/scanpy/preprocessing/_simple.py
Original file line number Diff line number Diff line change
Expand Up @@ -628,6 +628,21 @@
DT = TypeVar("DT")


@njit
def _create_regressor_categorical(
X: np.ndarray, cats: np.ndarray, 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)
Copy link
Contributor

Choose a reason for hiding this comment

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

check dtype for behavior with integer dtype i.e., need to ensure this is a floating point matrix

XT = X.T
for category in range(cats):
mask = category == filters
for ix in numba.prange(XT.shape[0]):
x = XT[ix]
regressors[mask, ix] = x[mask].mean()
return regressors

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

View check run for this annotation

Codecov / codecov/patch

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

Added lines #L636 - L643 were not covered by tests


@njit
def get_resid(
data: np.ndarray,
Expand Down Expand Up @@ -722,13 +737,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
cats = np.int64(len(adata.obs[keys[0]].cat.categories))
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto

Copy link
Contributor

Choose a reason for hiding this comment

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

Also comment why np.int64

Copy link
Member Author

Choose a reason for hiding this comment

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

because it has be done because of weird typing from pandas. So this ensures that it works within the kernel

Copy link
Member

Choose a reason for hiding this comment

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

so len doesn’t return a Python int? That’s a pandas bug.

filters = adata.obs[keys[0]].cat.codes.to_numpy()
cats = cats.astype(filters.dtype)

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, cats, 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.
42 changes: 42 additions & 0 deletions tests/test_preprocessing.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@
from scipy.sparse import coo_matrix, csc_matrix, csr_matrix, issparse

import scanpy as sc
from scanpy.preprocessing._simple import _create_regressor_categorical
from scanpy.preprocessing._utils import _to_dense
from testing.scanpy._helpers import (
anndata_v0_8_constructor_compat,
check_rep_mutation,
Expand Down Expand Up @@ -337,6 +339,46 @@ 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)


def _gen_org_regressors(adata, keys, X_org):
# helper function to generate the original regressors
regressors = np.zeros(X_org.shape, dtype=X_org.dtype)
X = _to_dense(X_org, order="F")
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()
return regressors


def test_regressor_categorical():
Copy link
Contributor

Choose a reason for hiding this comment

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

I would

  1. explain why this test exists (to test against a previous implementation? I am impartial whether it's necessary TBH since we are already testing for reproducibility, could see getting rid of this)
  2. refactor the "Create org regressors" into a helper function like create_original

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 can see your point here

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you have an an opinion on the first point? Is this test necessary? If so, perhaps a comment then?

adata = sc.datasets.pbmc68k_reduced()
adata = adata.raw.to_adata()[:200, :200]
X_org = adata.X.copy().astype(np.float64)
keys = ["bulk_labels"]
# Create org regressors
regressors = _gen_org_regressors(adata, keys, X_org)

# Create new regressors
cats = np.int64(len(adata.obs[keys[0]].cat.categories))
filters = adata.obs[keys[0]].cat.codes.to_numpy()
cats = cats.astype(filters.dtype)
X = _to_dense(X_org, order="F")
new_reg = _create_regressor_categorical(X, cats, filters)

# Compare the two implementations
np.testing.assert_allclose(new_reg, regressors)


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