-
Notifications
You must be signed in to change notification settings - Fork 616
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
DensMAP support #2946
base: main
Are you sure you want to change the base?
DensMAP support #2946
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2946 +/- ##
==========================================
+ Coverage 75.44% 75.49% +0.04%
==========================================
Files 113 114 +1
Lines 13250 13264 +14
==========================================
+ Hits 9997 10013 +16
+ Misses 3253 3251 -2
|
… into keller-mark/densmap-2
Hi, it seems the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, thanks for you contribution! Please add a plot test that shows sufficiently different results from umap.
src/scanpy/tools/_umap.py
Outdated
# Convenience function for densMAP | ||
|
||
|
||
def densmap( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’d rather not maintain that. Just method='densmap'
should be sufficient, but if densmap = partial(umap, method='densmap')
works and displays in the docs as expected, I’d consider it.
If you want to make it more visible, you could add an example to the sc.pl.umap
docs and maybe even a tutorial about what it’s for and how to best use it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried the partial
option, but there seem to be issues with inference of the types from the parent umap
function when generating the autodoc for sphinx that are not straightforward to resolve.
This test also fails, confirming the issue is not specific to DensMAP: 8f8787a |
Would be interesting to figure out which dependency causes this, then you could skip the image comparison if that dependency’s version is lower than a the offending version. But that could be difficult. If you’re not up to it, we could help or just give some marker for “this is the min-deps job” that you could use. |
@flying-sheep totally agree |
Thanks @keller-mark I think you're right for sure - it's not numpy 2 specific. The following requirements.txt causes the test to fail under python 3.10:
```
anndata==0.11.3
array-api-compat==1.10.0
asciitree==0.3.3
attrs==24.3.0
click==8.1.8
cloudpickle==3.1.1
contourpy==1.3.1
coverage==7.6.10
cycler==0.12.1
dask==2024.7.1
exceptiongroup==1.2.2
fasteners==0.19
fonttools==4.55.4
fsspec==2024.12.0
h5py==3.12.1
igraph==0.11.8
imageio==2.37.0
importlib-metadata==8.6.1
iniconfig==2.0.0
joblib==1.4.2
kiwisolver==1.4.8
lazy-loader==0.4
legacy-api-wrap==1.4.1
leidenalg==0.10.2
llvmlite==0.40.1
locket==1.0.0
matplotlib==3.10.0
natsort==8.4.0
networkx==3.4.2
numba==0.57.0
numcodecs==0.13.1
numpy==1.24.4
packaging==24.2
pandas==1.5.3
partd==1.4.2
patsy==1.0.1
pbr==6.1.0
pillow==11.1.0
pluggy==1.5.0
profimp==0.1.0
pynndescent==0.5.5
pyparsing==3.2.1
pytest==8.3.4
pytest-cov==6.0.0
pytest-mock==3.14.0
pytest-nunit==1.0.7
python-dateutil==2.9.0.post0
pytz==2024.2
pyyaml==6.0.2
scikit-image==0.22.0
scikit-learn==1.5.2
scipy==1.8.1
seaborn==0.13.2
session-info2==0.1.2
setuptools==75.8.0
six==1.17.0
statsmodels==0.14.4
texttable==1.7.0
threadpoolctl==3.5.0
tifffile==2025.1.10
tomli==2.2.1
toolz==1.0.0
tqdm==4.67.1
typing-extensions==4.12.2
tzdata==2025.1
umap-learn==0.5.1
zarr==2.18.3
zipp==3.21.0
-e file:///Users/ilangold/Projects/Theis/scanpy
```
But this one:
```
anndata==0.11.3
array-api-compat==1.10.0
asciitree==0.3.3
attrs==24.3.0
click==8.1.8
cloudpickle==3.1.1
contourpy==1.3.1
coverage==7.6.10
cycler==0.12.1
dask==2024.7.1
exceptiongroup==1.2.2
fasteners==0.19
fonttools==4.55.4
fsspec==2024.12.0
h5py==3.12.1
igraph==0.11.8
imageio==2.37.0
importlib-metadata==8.6.1
iniconfig==2.0.0
joblib==1.4.2
kiwisolver==1.4.8
lazy-loader==0.4
legacy-api-wrap==1.4.1
leidenalg==0.10.2
llvmlite==0.40.1
locket==1.0.0
matplotlib==3.10.0
natsort==8.4.0
networkx==3.4.2
numba==0.57.0
numcodecs==0.13.1
numpy==1.24.4
packaging==24.2
pandas==1.5.3
partd==1.4.2
patsy==1.0.1
pbr==6.1.0
pillow==11.1.0
pluggy==1.5.0
profimp==0.1.0
pynndescent==0.5.5
pyparsing==3.2.1
pytest==8.3.4
pytest-cov==6.0.0
pytest-mock==3.14.0
pytest-nunit==1.0.7
python-dateutil==2.9.0.post0
pytz==2024.2
pyyaml==6.0.2
scikit-image==0.22.0
scikit-learn==1.5.2
scipy==1.8.1
seaborn==0.13.2
session-info2==0.1.2
setuptools==75.8.0
six==1.17.0
statsmodels==0.14.4
texttable==1.7.0
threadpoolctl==3.5.0
tifffile==2025.1.10
tomli==2.2.1
toolz==1.0.0
tqdm==4.67.1
typing-extensions==4.12.2
tzdata==2025.1
umap-learn==0.5.1
zarr==2.18.3
zipp==3.21.0
-e file:///Users/ilangold/Projects/Theis/scanpy
```
causes the tests to pass (both installed as `uv pip install -r requirements.txt`).
So I've narrowed it down a bit...will start diffing now EDIT: sorry about the details, here are the files: |
Ok the issue is |
Specifically going from |
Thank you very much for debugging this @ilan-gold . Should I add a flag to skip this image comparison test with numba < 0.61.0? |
For now let's see if that makes things go green here and in the meantime I'll do some more digging. |
@keller-mark Something interesting is that |
@keller-mark I think that like most plotting tests in this repo (unfortunately), this test is actually erroneously passing with latest deps (including latest numba, for reasons I don't understand), at least for me locally. Here are actual/expected for me locally that "pass" as densemap: |
@keller-mark I'm curious if you're seeing locally what I'm seeing #2946 (comment) in the actual images produced. |
I do see the same actual/expected locally. I still cannot seem to find how to download the generated actual from Azure CI #2946 (comment) |
I think something along the lines of
worked for me. Have you tried this? Maybe it's a permissions thing? |
Thanks @ilan-gold for getting the image artifacts working on Azure CI. I now see that the actual/expected for both umap and densmap differ there despite passing, even in the Python 3.12 environment. I believe having easy access to DensMAP in Scanpy will be valuable to the community and would love to see this feature merged. |
I would be happy with a numerical test |
I have added a numerical test to check that the mean of ellipse areas that result from fitting a Gaussian mixture model with N components on the 2D coordinates from DensMAP and UMAP differ, with the DensMAP ellipses being larger on average than the UMAP ones. Where N = number of Louvain clusters. Let me know if this makes sense or if you have a better idea for a numerical test that will check that the UMAP and DensMAP results differ. |
I tried to follow the feedback described in a previous PR that contributed DensMAP #2684 (comment) but re-implemented on top of the state of the current scanpy main branch.
I did not add release notes because the contributor guide says to wait for PR feedback https://scanpy.readthedocs.io/en/latest/dev/documentation.html#adding-to-the-docs