Skip to content

fixed plot_factors_violin, removing deprecated "s","size" parameters #20

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

redst4r
Copy link

@redst4r redst4r commented Nov 6, 2024

Hi,

just noticed that the mofa.plot_factors_violin has a bug (might be relying on an older seaborn version) and plotting fails with a recent seaborn version (seaborn==0.13.2):

import mofax as mfx
mfx.plot_factors_violin(model, factors=0, dots=True, violins=False)

> AttributeError: PolyCollection.set() got an unexpected keyword argument 's'

It traces back to

File python3.11/site-packages/mofax/plot_factors.py:432, in plot_factors_violin(model, factors, color, violins, dots, zero_line, group_label, groups, linewidth, zero_linewidth, size, legend, legend_prop, palette, alpha, violins_alpha, ncols, sharex, sharey, **kwargs)
    424     modifier = partial(modifier, data=z)
    426 plot = partial(
    427     sns.violinplot,
    428     inner=None,
    429     s=size,
    430 )
--> 432 g = _plot_grid(
    433     plot,
    434     z,
    435     x="factor",
    436     y="value",
    437     color=color,
    438     zero_line_x=False,
    439     zero_line_y=zero_line,
    440     linewidth=linewidth,
    441     zero_linewidth=zero_linewidth,
    442     size=size,
    443     legend=legend,
    444     legend_prop=legend_prop,
    445     palette=palette,
    446     ncols=ncols,
    447     sharex=sharex,
    448     sharey=sharey,
    449     modifier=modifier,
    450     **kwargs,
    451 )

Turns out sns.violinplot doesn't accept s or size as an agument (any more?).
Removing those two lines (429 and 442) fixes the issue. Dotsize is still adjustable if desired:

mfx.plot_factors_violin(model, factors=0, dots=True, violins=False, size=10)

@quentinblampey
Copy link

quentinblampey commented Feb 7, 2025

I had the same issue and came out with the same fix (before seing this PR...)

I quickly checked the seaborn versions, and plot_factors_violin works only with seaborn<0.13.0, i.e. any version released before September 2023.
I think you can safely accept the PR because scanpy depends on seaborn>=0.13.0 anyway, so using seaborn>=0.13.0 in mofax shouldn't introduce any dependency change.

What do you think @gtca?

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.

2 participants