Skip to content

refactor: normalize_total with Numba - #3593

Merged
flying-sheep merged 5 commits into
scverse:normalize_total_updatefrom
selmanozleyen:integrated_normalize_update
Apr 15, 2025
Merged

refactor: normalize_total with Numba#3593
flying-sheep merged 5 commits into
scverse:normalize_total_updatefrom
selmanozleyen:integrated_normalize_update

Conversation

@selmanozleyen

@selmanozleyen selmanozleyen commented Apr 15, 2025

Copy link
Copy Markdown
Member

Hi this PR integrates the recent commits and refactors on #3571.

Here I removed the division part from the numba code so that it is unified in the future with fast-array-utils elem_mult.

pinging bc I can't ask for reviews for some reason: @flying-sheep

@flying-sheep
flying-sheep self-requested a review April 15, 2025 13:02
flying-sheep

This comment was marked as resolved.

@selmanozleyen

Copy link
Copy Markdown
Member Author

@flying-sheep ok I see what you wanted but I thought you wanted to squash my merge into Severin's branch then review on that branch. I am on it

@flying-sheep

Copy link
Copy Markdown
Member

well, then do the same with severin’s branch instead of main.

@selmanozleyen
selmanozleyen force-pushed the integrated_normalize_update branch from cd7ab7d to a3e5cf0 Compare April 15, 2025 13:22
@selmanozleyen
selmanozleyen changed the base branch from normalize_total_update to main April 15, 2025 13:27
@selmanozleyen

Copy link
Copy Markdown
Member Author

ok I did change the base to main in the PR so that there is only two files to review.

@flying-sheep
flying-sheep force-pushed the integrated_normalize_update branch from a3e5cf0 to ce6cf62 Compare April 15, 2025 13:48
@flying-sheep
flying-sheep changed the base branch from main to normalize_total_update April 15, 2025 13:49

@flying-sheep flying-sheep left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good! I fixed an issue with the logging (time measuring was broken), otherwise this looks good as-is!

Nice deduplication also of the not immediately obvious parts x.copy() and x.astype(np.float32)

Comment on lines +282 to +283
if not inplace:
x = x.copy()

@flying-sheep flying-sheep Apr 15, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

converting a csc to a csr matrix will copy already, but I guess an extra copy in that case isn’t too bad.

(I get that we also need to enter this path for non-csparse x)

Comment on lines +85 to +86

It returns the normalized data, the counts per cell, and the gene subset.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
It returns the normalized data, the counts per cell, and the gene subset.

No need to say that twice

@flying-sheep
flying-sheep merged commit b27d218 into scverse:normalize_total_update Apr 15, 2025
ilan-gold added a commit that referenced this pull request May 19, 2025
* update normalize_total & remove dep

* refactor: `normalize_total` with Numba (#3593)

Co-authored-by: Philipp A. <flying-sheep@web.de>

* fix doctest

* add release note

* Switch from profimp to tuna for parsing import profiles (#3620)

* [pre-commit.ci] pre-commit autoupdate (#3622)

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Philipp A. <flying-sheep@web.de>

* Fix typo (#3555)

Co-authored-by: Phil Schaf <flying-sheep@web.de>

* Switch to fast-array-utils (#3598)

Co-authored-by: Rodrigo Goya <rgoya@users.noreply.github.com>

* tSNE components parameter (#2803)

Co-authored-by: Phil Schaf <flying-sheep@web.de>

* Simplify scale (#3351)

Co-authored-by: Intron7 <severin.dicks@icloud.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

* remove layers test

* explicit return and fix the info message

* remove unnecessary ravel's

* precision fix

* (fix): maintain data type properly.

* (fix): remove artifact storage

* (refactor): use nice f-a-u typing

---------

Co-authored-by: Philipp A. <flying-sheep@web.de>
Co-authored-by: Selman Özleyen <32667648+selmanozleyen@users.noreply.github.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Swastik Mishra <4453983+swstkm@users.noreply.github.com>
Co-authored-by: Rodrigo Goya <rgoya@users.noreply.github.com>
Co-authored-by: Kitsune <48340051+ch1ru@users.noreply.github.com>
Co-authored-by: selmanozleyen <syozleyen@gmail.com>
Co-authored-by: ilan-gold <ilanbassgold@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants