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 28 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
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`
31 changes: 25 additions & 6 deletions src/scanpy/preprocessing/_simple.py
Original file line number Diff line number Diff line change
Expand Up @@ -637,6 +637,22 @@
DT = TypeVar("DT")


@njit
def _create_regressor_categorical(
X: np.ndarray, number_categories: int, cat_array: np.ndarray
) -> np.ndarray:
# create regressor matrix for categorical variables
regressors = np.zeros(X.shape, dtype=X.dtype)

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

View check run for this annotation

Codecov / codecov/patch

src/scanpy/preprocessing/_simple.py#L645

Added line #L645 was not covered by tests
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

# iterate over categories
for category in range(number_categories):

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

View check run for this annotation

Codecov / codecov/patch

src/scanpy/preprocessing/_simple.py#L647

Added line #L647 was not covered by tests
# iterate over genes and calculate mean expression
# for each gene per category
mask = category == cat_array
for ix in numba.prange(X.T.shape[0]):
regressors[mask, ix] = X.T[ix, mask].mean()
return regressors

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

View check run for this annotation

Codecov / codecov/patch

src/scanpy/preprocessing/_simple.py#L650-L653

Added lines #L650 - L653 were not covered by tests


@njit
def get_resid(
data: np.ndarray,
Expand Down Expand Up @@ -732,13 +748,16 @@
)
raise ValueError(msg)
logg.debug("... regressing on per-gene means within categories")
regressors = np.zeros(X.shape, dtype="float32")

# set number of categories to the same dtype as the categories
cat_array = adata.obs[keys[0]].cat.codes.to_numpy()
number_categories = cat_array.dtype.type(len(adata.obs[keys[0]].cat.categories))

X = _to_dense(X, order="F") if issparse(X) else X
ilan-gold marked this conversation as resolved.
Show resolved Hide resolved
# 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()
if np.issubdtype(X.dtype, np.integer):
target_dtype = np.float32 if X.dtype.itemsize == 4 else np.float64
X = X.astype(target_dtype)

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

View check run for this annotation

Codecov / codecov/patch

src/scanpy/preprocessing/_simple.py#L758-L759

Added lines #L758 - L759 were not covered by tests
regressors = _create_regressor_categorical(X, number_categories, cat_array)
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.
17 changes: 12 additions & 5 deletions tests/test_preprocessing.py
Original file line number Diff line number Diff line change
Expand Up @@ -464,14 +464,21 @@ def test_regress_out_constants():
assert_equal(adata, adata_copy)


def test_regress_out_reproducible():
adata = pbmc68k_reduced()
@pytest.mark.parametrize(
("keys", "test_file", "atol"),
[
(["n_counts", "percent_mito"], "regress_test_small.npy", 0.0),
(["bulk_labels"], "regress_test_small_cat.npy", 1e-6),
],
)
def test_regress_out_reproducible(keys, test_file, atol):
adata = sc.datasets.pbmc68k_reduced()
adata = adata.raw.to_adata()[:200, :200].copy()
sc.pp.regress_out(adata, keys=["n_counts", "percent_mito"])
sc.pp.regress_out(adata, keys=keys)
# 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.npy")
np.testing.assert_allclose(adata.X, tester)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the change in testing function? There's a pretty big note (see https://numpy.org/doc/stable/reference/generated/numpy.testing.assert_array_almost_equal.html) about why we should use assert_allclose in a floating point situation, which I believe is what we're in right now.

tester = np.load(DATA_PATH / test_file)
np.testing.assert_allclose(adata.X, tester, atol=atol)


def test_regress_out_constants_equivalent():
Expand Down