Skip to content
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

Bug in viz.boxplots function (and possibly others) #95

Open
reemagit opened this issue Feb 25, 2024 · 3 comments
Open

Bug in viz.boxplots function (and possibly others) #95

reemagit opened this issue Feb 25, 2024 · 3 comments

Comments

@reemagit
Copy link

Hello,

I think I found a bug in the viz.boxplots function, and also applied a solution that worked. Explaining it here in case other people ever ran into this problem.

I have a cell counts AnnData object where each row is a subject and each column are the cell counts for that subject.
For each subjects I have several categorical features such as Smoke that can assume three values and Disease that assumes two values.

I first plot

viz.boxplots(data_coda, feature_name = "Disease")

This works. Then, I plot

viz.boxplots(data_coda, feature_name = "Smoke")

which also works. However, if I plot the feature "Disease" again, it crashes with the following error:

---------------------------------------------------------------------------
UnboundLocalError                         Traceback (most recent call last)
Cell In[73], line 1
----> 1 viz.boxplots(data_coda,'GOLD')

File ~/miniforge3/envs/polvcopd4/lib/python3.9/site-packages/sccoda/util/data_visualization.py:319, in boxplots(data, feature_name, y_scale, plot_facets, add_dots, cell_types, args_boxplot, args_swarmplot, figsize, dpi, cmap, plot_legend, level_order)
    315     args_swarmplot["hue_order"] = level_order
    317 fig, ax = plt.subplots(figsize=figsize, dpi=dpi)
--> 319 sns.boxplot(x="Cell type", y=value_name, hue=feature_name, data=plot_df, fliersize=1,
    320             palette=cmap, ax=ax, **args_boxplot)
    322 if add_dots:
    323     sns.swarmplot(
    324         x="Cell type",
    325         y=value_name,
   (...)
    331         **args_swarmplot
    332     )

File ~/miniforge3/envs/polvcopd4/lib/python3.9/site-packages/seaborn/categorical.py:1634, in boxplot(data, x, y, hue, order, hue_order, orient, color, palette, saturation, fill, dodge, width, gap, whis, linecolor, linewidth, fliersize, hue_norm, native_scale, log_scale, formatter, legend, ax, **kwargs)
   1627 color = _default_color(
   1628     ax.fill_between, hue, color,
   1629     {k: v for k, v in kwargs.items() if k in ["c", "color", "fc", "facecolor"]},
   1630     saturation=saturation,
   1631 )
   1632 linecolor = p._complement_color(linecolor, color, p._hue_map)
-> 1634 p.plot_boxes(
   1635     width=width,
   1636     dodge=dodge,
   1637     gap=gap,
   1638     fill=fill,
   1639     whis=whis,
   1640     color=color,
   1641     linecolor=linecolor,
   1642     linewidth=linewidth,
   1643     fliersize=fliersize,
   1644     plot_kws=kwargs,
   1645 )
   1647 p._add_axis_labels(ax)
   1648 p._adjust_cat_axis(ax, axis=p.orient)

File ~/miniforge3/envs/polvcopd4/lib/python3.9/site-packages/seaborn/categorical.py:745, in _CategoricalPlotter.plot_boxes(self, width, dodge, gap, fill, whis, color, linecolor, linewidth, fliersize, plot_kws)
    742     ax.add_container(BoxPlotContainer(artists))
    744 legend_artist = _get_patch_legend_artist(fill)
--> 745 self._configure_legend(ax, legend_artist, boxprops)

UnboundLocalError: local variable 'boxprops' referenced before assignment

Note that this behavior is not easy to reproduce since I think it depends on how many categories each feature has. Moreover, when plotting other features, I found that the legend contained the levels of the previous feature "Smoke". This means that some global variables are persisting between function calls and are leaking in subsequent calls. I think I reconstructed why this happens. In the data_visualization.py script, the function signature of the boxplots function is defined as

def boxplots(
        data: AnnData,
        feature_name: str,
        y_scale: str = "relative",
        plot_facets: bool = False,
        add_dots: bool = False,
        cell_types: Optional[list] = None,
        args_boxplot: Optional[dict] = {},
        args_swarmplot: Optional[dict] = {},
        figsize: Optional[Tuple[int, int]] = None,
        dpi: Optional[int] = 100,
        cmap: Optional[str] = "Blues",
        plot_legend: Optional[bool] = True,
        level_order: List[str] = None
) -> Optional[Tuple[plt.Subplot, sns.axisgrid.FacetGrid]]:

This function signature defines the default arguments of args_boxplot and args_swamplot as empty dicts ({}). Using mutable default values is generally not recommended in python because it causes unexpected behaviors between function calls (see here).
To avoid this behavior, I just replaced the default values with None and filled them within the function:

def boxplots(
        data: AnnData,
        feature_name: str,
        y_scale: str = "relative",
        plot_facets: bool = False,
        add_dots: bool = False,
        cell_types: Optional[list] = None,
        args_boxplot: Optional[dict] = None,
        args_swarmplot: Optional[dict] = None,
        figsize: Optional[Tuple[int, int]] = None,
        dpi: Optional[int] = 100,
        cmap: Optional[str] = "Blues",
        plot_legend: Optional[bool] = True,
        level_order: List[str] = None
) -> Optional[Tuple[plt.Subplot, sns.axisgrid.FacetGrid]]:

and inside the function we add

if args_swarmplot is None:
    args_swarmplot = {}
if args_boxplot is None:
    args_boxplot = {}

Not sure if other functions use mutable default values. Thought it could be useful if other people ever ran into this problem.

@johannesostner
Copy link
Collaborator

Thanks for spotting this!
I'll keep this fix in mind if we ever release a new version of this package. The actively maintained implementation of scCODA in the pertpy package already contains a very similar fix.

@reemagit
Copy link
Author

I didn't know about the pertpy package, I'll check it out, thanks for letting me know. Not sure if I should close the issue or leave it open?

@johannesostner
Copy link
Collaborator

I'll leave it open for now, so that I won't forget about this fix.
Thanks again!

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

No branches or pull requests

2 participants