Skip to content

Fix #755: calculate marker positions from DataFrame, not rendered labels#970

Open
kimjune01 wants to merge 3 commits into
scverse:mainfrom
kimjune01:fix-plot-multicomparison-fc-755
Open

Fix #755: calculate marker positions from DataFrame, not rendered labels#970
kimjune01 wants to merge 3 commits into
scverse:mainfrom
kimjune01:fix-plot-multicomparison-fc-755

Conversation

@kimjune01
Copy link
Copy Markdown
Contributor

@kimjune01 kimjune01 commented May 12, 2026

Follow-up to #965 (merged). Supersedes #966 (closed due to branch recreation during rebase).

#965 fixed the immediate crash by forcing tick labels on, but this still breaks if a user passes xticklabels=False via heatmap_kwargs. This PR removes the dependency on rendered tick labels entirely.

Before:
Before

After:
After

Changes:

  • Calculate marker positions directly from DataFrame structure (cell centers at 0.5, 1.5, ...) instead of extracting from rendered plot
  • Works regardless of label visibility, figsize, or gene count
  • Add comment noting that positioning assumes non-clustered heatmap
  • Update test to reproduce the original bug (50 genes + small figsize) and test with explicitly hidden labels

kimjune01 added 2 commits May 12, 2026 02:15
…red labels

Previous approach extracted tick labels from the rendered plot, which failed
when seaborn hid labels (small figsize, many genes, or explicit xticklabels=False).

New approach calculates positions directly from DataFrame structure:
- Seaborn places cell centers at 0.5, 1.5, 2.5, etc.
- Works regardless of label visibility
- Faster (no DOM extraction)
- Immune to matplotlib rendering changes

Test updated to actually reproduce the bug (50 genes + small figsize).
Gemini review noted that position calculation assumes non-clustered heatmap.
If future maintainer changes to clustermap, marker positions would be wrong.
Added comment to make this assumption explicit.
@kimjune01
Copy link
Copy Markdown
Contributor Author

Rebased onto main, conflicts resolved. This supersedes #966 which was closed during the rebase.

The approach here is different from #965: instead of forcing tick labels on (which breaks if a user explicitly hides them), this calculates marker positions directly from the DataFrame. Works regardless of label visibility.

@Zethson
Copy link
Copy Markdown
Member

Zethson commented May 12, 2026

There's test failures now:

FAILED tests/tools/_differential_gene_expression/test_base.py::test_plot_multicomparison_fc_many_genes[csr_matrix] - AttributeError: QuadMesh.set() got an unexpected keyword argument 'heatmap_kwargs'
FAILED tests/tools/_differential_gene_expression/test_base.py::test_plot_multicomparison_fc_many_genes[array] - AttributeError: QuadMesh.set() got an unexpected keyword argument 'heatmap_kwargs'
FAILED tests/tools/_differential_gene_expression/test_base.py::test_plot_multicomparison_fc_many_genes[csc_matrix] - AttributeError: QuadMesh.set() got an unexpected keyword argument 'heatmap_kwargs'

could you please also ensure that LLM comments that aren't helpful and/or noisy are kept to a minimum?
Could you please do me a favor and show a before/after plot? Ideally also a minimum reproducible example that I can quickly paste to understand the effect of these changes.

Thank you!

plot_multicomparison_fc uses **heatmap_kwargs, so xticklabels
must be passed as a keyword argument, not inside a dict.
@kimjune01
Copy link
Copy Markdown
Contributor Author

kimjune01 commented May 12, 2026

Fixed. All 3 parametrized variants pass.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented May 12, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 71.85%. Comparing base (12897e1) to head (4107afc).
⚠️ Report is 58 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #970      +/-   ##
==========================================
- Coverage   73.54%   71.85%   -1.69%     
==========================================
  Files          48       48              
  Lines        5613     5731     +118     
==========================================
- Hits         4128     4118      -10     
- Misses       1485     1613     +128     
Files with missing lines Coverage Δ
...ertpy/tools/_differential_gene_expression/_base.py 90.24% <ø> (+1.21%) ⬆️

... and 7 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Zethson
Copy link
Copy Markdown
Member

Zethson commented May 15, 2026

Could you please also ensure that LLM comments that aren't helpful and/or noisy are kept to a minimum?
Could you please do me a favor and show a before/after plot? Ideally also a minimum reproducible example that I can quickly paste to understand the effect of these changes.

Could you please address these comments? Thank you!

@kimjune01
Copy link
Copy Markdown
Contributor Author

kimjune01 commented May 16, 2026

The new test_plot_multicomparison_fc_many_genes in this PR is the repro. The fix computes them from the DataFrame, so it works regardless. Before/after plots are in the description.

The two red py3.14-pre checks aren't from this branch. the same four tests/tools/_coda/ tests have been failing on main since #965 merged. @Zethson

@Zethson
Copy link
Copy Markdown
Member

Zethson commented May 16, 2026

Thanks! Don't worry about the failing unrelated tests.

Am I stupid or is the before & after switched? We'd expect to see the x-tick labels right?

@kimjune01
Copy link
Copy Markdown
Contributor Author

kimjune01 commented May 16, 2026

@Zethson silly mistake 🤪

Also added ! for easier viewing

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.

3 participants