mofax icon indicating copy to clipboard operation
mofax copied to clipboard

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

Open redst4r opened this issue 1 year ago • 1 comments

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)

redst4r avatar Nov 06 '24 23:11 redst4r

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?

quentinblampey avatar Feb 07 '25 10:02 quentinblampey