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

Use numpy histogram in scatter_density #70

Merged
merged 5 commits into from
May 20, 2024
Merged

Conversation

quentinblampey
Copy link
Contributor

@quentinblampey quentinblampey commented May 17, 2024

Hello,

These are the changes related to #64, i.e. about the usage of np.histogram2d instead of datashader in scatter_density: the performances are the same, as shown below.

I kept the same function signature, except for cmap where str | Colormap is supported, but not List yet (do we often need to use a list of colors as a cmap?)

Since datashader is not used in pytometry anymore, I removed it from the dependencies (as long as it's not needed for another plot), and I also moved nbproject in the test dependencies.

I also added a CHANGELOG.md file, I think it's a good practice to have this to keep track of the changes per version, what do you think @mbuttner, @grst?

With datashader

%%time
pm.pl.scatter_density(adata, x="CD4", y="CD8")
CPU times: user 494 ms, sys: 7.36 ms, total: 501 ms
Wall time: 210 ms

image

With numpy histogram2d

%%time
pm.pl.scatter_density(adata, x="CD4", y="CD8")
CPU times: user 403 ms, sys: 7.67 ms, total: 411 ms
Wall time: 203 ms

image

Copy link

github-actions bot commented May 17, 2024

@github-actions github-actions bot temporarily deployed to pull request May 17, 2024 09:41 Inactive
Copy link
Collaborator

@grst grst left a comment

Choose a reason for hiding this comment

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

LGTM, just one comment RE changelog.

CHANGELOG.md Outdated Show resolved Hide resolved
@github-actions github-actions bot temporarily deployed to pull request May 17, 2024 12:56 Inactive
@mbuttner
Copy link
Collaborator

Looks good to me, too!

@@ -41,6 +39,7 @@ dev = [
test = [
"pytest>=6.0",
"pytest-cov",
"nbproject",
Copy link
Member

Choose a reason for hiding this comment

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

To be honest, I'm not sure why you need nbproject at all. For tests, you only need nbproject_test.

It's used once in the code:

from nbproject._logger import logger (https://github.com/scverse/pytometry/blob/main/tests/test_notebooks.py#L4) and this can easily be replaced with just a print or Python logger statement

@@ -54,6 +53,8 @@ def scatter_density(
draw into an existing figure.
figsize (tuple), optional:
Figure size (width, height) if ``ax`` not provided. Defaults to (10, 10).
bins (int or tuple), optional:
Copy link
Member

Choose a reason for hiding this comment

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

This is a general point, but I don't see the point of typing docstrings when you're using typehints in the function stub (as you should!). It's annoying to keep both in sync. Sphinx picks up the types from the function header anyways.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I totally agree, I'll open a separate issue!

@mbuttner mbuttner merged commit 8e74529 into main May 20, 2024
2 checks passed
@mbuttner mbuttner deleted the scatter_density_numpy branch May 20, 2024 16:02
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.

4 participants