From ae35bc11b6163da0547acdf462a1f1b0220d4142 Mon Sep 17 00:00:00 2001 From: June Kim Date: Mon, 11 May 2026 15:56:25 -0700 Subject: [PATCH 1/3] Fix #755: calculate marker positions from DataFrame, not rendered 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). --- .../_differential_gene_expression/_base.py | 14 +++++----- .../test_base.py | 27 +++++++++++-------- 2 files changed, 23 insertions(+), 18 deletions(-) diff --git a/pertpy/tools/_differential_gene_expression/_base.py b/pertpy/tools/_differential_gene_expression/_base.py index d879c61d..5339e31f 100644 --- a/pertpy/tools/_differential_gene_expression/_base.py +++ b/pertpy/tools/_differential_gene_expression/_base.py @@ -836,15 +836,15 @@ def _get_significance(p_val): df = results_df.pivot(index=contrast_col, columns=symbol_col, values=log2fc_col)[var_names] plt.figure(figsize=figsize) - # Ensure tick labels are shown by default (fixes #755) - # Users can override via heatmap_kwargs - default_heatmap_kwargs = {"xticklabels": True, "yticklabels": True} - default_heatmap_kwargs.update(heatmap_kwargs) - sns.heatmap(df, **default_heatmap_kwargs, cmap="coolwarm", center=0, cbar_kws={"label": "Log2 fold change"}) + sns.heatmap(df, **heatmap_kwargs, cmap="coolwarm", center=0, cbar_kws={"label": "Log2 fold change"}) _size = {"< 0.001": marker_size, "< 0.01": math.floor(marker_size / 2), "< 0.1": math.floor(marker_size / 4)} - x_locs, x_labels = plt.xticks()[0], [label.get_text() for label in plt.xticks()[1]] - y_locs, y_labels = plt.yticks()[0], [label.get_text() for label in plt.yticks()[1]] + # Calculate locations directly from DataFrame instead of extracting from rendered plot (fixes #755) + # Seaborn places cell centers at 0.5, 1.5, 2.5, etc. + x_locs = np.arange(len(df.columns)) + 0.5 + x_labels = df.columns.tolist() + y_locs = np.arange(len(df.index)) + 0.5 + y_labels = df.index.tolist() for _i, row in results_df.iterrows(): if row["significance"] != "n.s.": diff --git a/tests/tools/_differential_gene_expression/test_base.py b/tests/tools/_differential_gene_expression/test_base.py index a1bd2130..1e367585 100644 --- a/tests/tools/_differential_gene_expression/test_base.py +++ b/tests/tools/_differential_gene_expression/test_base.py @@ -92,16 +92,17 @@ def test_model_cond(test_adata_minimal, MockLinearModel, formula, cond_kwargs, e assert actual_contrast.index.tolist() == mod.design.columns.tolist() -def test_plot_multicomparison_fc_default_figsize(MockLinearModel, test_adata_minimal): - """Test that plot_multicomparison_fc works with default figsize. +def test_plot_multicomparison_fc_many_genes(MockLinearModel, test_adata_minimal): + """Test that plot_multicomparison_fc works even when heatmap hides tick labels. Regression test for issue #755. - When using default figsize, seaborn heatmap may not show xticklabels, - causing a ValueError when trying to plot significance markers. + When using small figsize or many genes, seaborn heatmap hides xticklabels. + The old code extracted labels from the rendered plot, causing ValueError. + The fix calculates positions directly from the DataFrame. """ - # Create mock results similar to what compare_groups would return + # Create mock results with many genes to force label hiding results = [] - genes = ["GENE1", "GENE2", "GENE3", "IL5", "IL6", "IL10"] + genes = [f"GENE{i}" for i in range(50)] # 50 genes will force label hiding contrasts = ["contrast1", "contrast2"] for contrast in contrasts: @@ -110,8 +111,8 @@ def test_plot_multicomparison_fc_default_figsize(MockLinearModel, test_adata_min { "contrast": contrast, "variable": gene, - "log_fc": 1.5 + i * 0.3, - "adj_p_value": 0.001 if i < 3 else 0.05, + "log_fc": 1.5 + i * 0.05, + "adj_p_value": 0.001 if i < 10 else 0.05, } ) @@ -120,7 +121,11 @@ def test_plot_multicomparison_fc_default_figsize(MockLinearModel, test_adata_min # Create a mock model instance mod = MockLinearModel(test_adata_minimal, "~condition") - # This should not raise ValueError: 'IL5' is not in list - # even with default figsize (which may cause heatmap to hide labels) - fig = mod.plot_multicomparison_fc(results_df, return_fig=True) + # This should not raise ValueError even with small figsize + # that causes seaborn to hide tick labels + fig = mod.plot_multicomparison_fc(results_df, figsize=(6, 4), return_fig=True) + assert fig is not None + + # Also test with heatmap_kwargs that explicitly hide labels + fig = mod.plot_multicomparison_fc(results_df, heatmap_kwargs={"xticklabels": False}, return_fig=True) assert fig is not None From b4d036a82494c230017b38a65507106c638b7a3e Mon Sep 17 00:00:00 2001 From: June Kim Date: Mon, 11 May 2026 17:53:12 -0700 Subject: [PATCH 2/3] Add clustermap warning comment 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. --- pertpy/tools/_differential_gene_expression/_base.py | 1 + 1 file changed, 1 insertion(+) diff --git a/pertpy/tools/_differential_gene_expression/_base.py b/pertpy/tools/_differential_gene_expression/_base.py index 5339e31f..a2f4c2ce 100644 --- a/pertpy/tools/_differential_gene_expression/_base.py +++ b/pertpy/tools/_differential_gene_expression/_base.py @@ -841,6 +841,7 @@ def _get_significance(p_val): _size = {"< 0.001": marker_size, "< 0.01": math.floor(marker_size / 2), "< 0.1": math.floor(marker_size / 4)} # Calculate locations directly from DataFrame instead of extracting from rendered plot (fixes #755) # Seaborn places cell centers at 0.5, 1.5, 2.5, etc. + # NOTE: This assumes a non-clustered heatmap. If using clustermap, coordinates would need reordering. x_locs = np.arange(len(df.columns)) + 0.5 x_labels = df.columns.tolist() y_locs = np.arange(len(df.index)) + 0.5 From 4107afca9f2c05880f574361d09272e4301029bc Mon Sep 17 00:00:00 2001 From: June Kim Date: Tue, 12 May 2026 06:31:27 -0700 Subject: [PATCH 3/3] Fix test: pass heatmap kwargs directly, not wrapped in dict plot_multicomparison_fc uses **heatmap_kwargs, so xticklabels must be passed as a keyword argument, not inside a dict. --- tests/tools/_differential_gene_expression/test_base.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/tools/_differential_gene_expression/test_base.py b/tests/tools/_differential_gene_expression/test_base.py index 1e367585..50bc0b5f 100644 --- a/tests/tools/_differential_gene_expression/test_base.py +++ b/tests/tools/_differential_gene_expression/test_base.py @@ -127,5 +127,5 @@ def test_plot_multicomparison_fc_many_genes(MockLinearModel, test_adata_minimal) assert fig is not None # Also test with heatmap_kwargs that explicitly hide labels - fig = mod.plot_multicomparison_fc(results_df, heatmap_kwargs={"xticklabels": False}, return_fig=True) + fig = mod.plot_multicomparison_fc(results_df, xticklabels=False, return_fig=True) assert fig is not None