Skip to content

Conversation

@keller-mark
Copy link

@keller-mark keller-mark commented Mar 22, 2024

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

  • Release notes not necessary because:

@codecov
Copy link

codecov bot commented Mar 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.44%. Comparing base (607bb9c) to head (019035d).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2946      +/-   ##
==========================================
+ Coverage   75.40%   75.44%   +0.04%     
==========================================
  Files         113      114       +1     
  Lines       13259    13273      +14     
==========================================
+ Hits         9998    10014      +16     
+ Misses       3261     3259       -2     
Files with missing lines Coverage Δ
src/scanpy/tools/__init__.py 91.30% <ø> (ø)
src/scanpy/tools/_types.py 100.00% <100.00%> (ø)
src/scanpy/tools/_umap.py 75.71% <100.00%> (+6.35%) ⬆️

@keller-mark keller-mark changed the title DensMAP support 2 DensMAP support Dec 10, 2024
@keller-mark
Copy link
Author

Hi, it seems the Check if milestone or "no milestone" label is present GH Actions check fails but I am not able to add a PR label

Copy link
Member

@flying-sheep flying-sheep left a 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.

@flying-sheep flying-sheep added this to the 1.11.0 milestone Dec 16, 2024
@ilan-gold
Copy link
Contributor

@keller-mark I'm curious if you're seeing locally what I'm seeing #2946 (comment) in the actual images produced.

@keller-mark
Copy link
Author

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)

@ilan-gold
Copy link
Contributor

I think something along the lines of

  - task: PublishBuildArtifacts@1
    inputs:
      pathToPublish: '$(Pipeline.Workspace)/s/scanpy/tests/_images'
      artifactName: drop

worked for me. Have you tried this? Maybe it's a permissions thing?

@keller-mark
Copy link
Author

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.
Based on the discussion in #3422 (comment) I think it is unreasonable to expect random results, even with the same seed, to match between my local environment and the CI environment. Therefore I reiterate my earlier request to replace the visual test with a numerical test #2946 (comment) Alternatively, we could store a precomputed DensMAP result and plot that, but this would just test the sc.pl.embedding code path which is already tested and untouched in this PR.

@ilan-gold
Copy link
Contributor

Since ultimately the plotting is just using the standard sc.pl.embedding, is a visual test really necessary here? That code path is already extensively tested. Instead I could add a numerical (as opposed to visual) test that checks that the method="densmap" produces sufficiently different results than method="umap"

I would be happy with a numerical test

@keller-mark
Copy link
Author

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.

@ilan-gold
Copy link
Contributor

@keller-mark So the idea here is that a densemap always has slightly larger ellipses than UMAP, but is otherwise the same? This is the claim of the method? If so, the test looks alright to me

Copy link
Contributor

@ilan-gold ilan-gold left a comment

Choose a reason for hiding this comment

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

Looking good

Copy link
Contributor

@ilan-gold ilan-gold left a comment

Choose a reason for hiding this comment

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

I like the test - it says "this should look on average a certain way across different machines" which is about as good as we can hope for IMO. Unless there's something else, we can merge

@keller-mark
Copy link
Author

Recent preprint discussing the benefits of DensMAP: https://doi.org/10.48550/arXiv.2506.08725

@keller-mark
Copy link
Author

I have gotten this PR passing CI a few times but the underlying repo keeps changing out from under me, which is quite frustrating. I know it is a difficult problem and maintainers are spread thin but I truly believe having DensMAP functionality in Scanpy would benefit many users.

@flying-sheep flying-sheep modified the milestones: 1.12.0, 1.13.0 Jan 23, 2026
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.

densmap

3 participants