Skip to content

Fix / Unit tests test_violin and test_pbmc3k #1422

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Sep 22, 2020
Merged

Fix / Unit tests test_violin and test_pbmc3k #1422

merged 6 commits into from
Sep 22, 2020

Conversation

@WeilerP
Copy link
Contributor

@WeilerP WeilerP commented Sep 17, 2020

Description

As briefly outlined in #1420, using seaborn==0.11 causes the unit tests scanpy/tests/test_plotting.py:test_violin and scanpy/tests/notebooks/test_pbmc3k.py fail as the violin plots are no longer vertically oriented.

Changes

  • Rely on seaborn.catplot and seaborn.stripplot instead of seaborn.FacetGrid in scanpy/plotting/_anndata.py

Related issues

Closes #1420.

@WeilerP
Update _anndata.py
9ec9f19 
When using `seaborn==0.11`, the vertical orientation is ignored when
only `x` is specified. To orientate the violin plot vertically,
`seaborn.catplot` can be used instead of `seaborn.FacetGrid`.
The keyword arguments `"cut"` and `"inner"` cannot be passed to
`seaborn.catplot` or `seaborn.stripplot` and are thus moved to the
`else` clause.
@WeilerP
Update master_violin_multi_panel.png
5364c3d 
Following the changes in `_anndata.py`, the PNG file generated by
`test_plotting.py:test_violin` has to be updated.
@WeilerP
Update violin.png
3d1c6c9 
Following the changes in `_anndata.py`, the PNG file generated by
`test_pbmc3k.py:test_pbmc3k` has to be updated.
@WeilerP
Copy link
Contributor Author

WeilerP commented Sep 17, 2020

@fidelram, from seaborn==0.12 on, the only valid positional argument will be data which may cause failures or unexpected behaviour. This is relevant at least here and here. Should we add / specify the keyword argument x in this PR or open a separate issue and PR?

@fidelram
Copy link
Collaborator

fidelram commented Sep 18, 2020

@WeilerP Thanks for finding about this. Yes, please add explicitly x=x in this PR 👍

@WeilerP
Update _anndata.py
5099e46 
Pass `x` as a keyword argument to `seaborn.violinplot` and
`seaborn.stripplot`.
This supresses seaborn's FutureWarning "From version 0.12, the only
valid positional argument will be `data`, and passing other arguments
without an explicit keyword will result in an error or
misinterpretation."
@fidelram
Copy link
Collaborator

fidelram commented Sep 18, 2020

Thanks. Any idea why the images are now bigger (which I assume is the reason for the very small fonts)?

@WeilerP
Update _anndata.py
f2c53a8 
Pass keyword arguments `scale`, `cut` and `inner` to `seaborn.catplot`.
@WeilerP
Copy link
Contributor Author

WeilerP commented Sep 18, 2020

Hm, no sorry, I don't. My best guess would be that seaborn.catplot and seaborn.FacetGrid use different default figure sizes which in turn makes the font look smaller?

@WeilerP
Copy link
Contributor Author

WeilerP commented Sep 18, 2020

I noticed, however, that the tests pass even when removing the if stripplot: part. Any idea on why this is happening and how to prevent it?

@fidelram
Copy link
Collaborator

fidelram commented Sep 18, 2020

The tests have a tolerance parameter that is set high. The problem is that the stripplot shows different results each time. Also, different versions of matplotlib and seaborn have slight differences.

@fidelram
Copy link
Collaborator

fidelram commented Sep 22, 2020

I wonder why the tests are not working now?

@WeilerP
Update violin.png
ac25a6d 
Following the latest changes to `_anndata.py`, the expected PNG file
generated by `test_pbmc3k.py:test_pbmc3k` is updated.
@WeilerP
Copy link
Contributor Author

WeilerP commented Sep 22, 2020

I wonder why the tests are not working now?

Sorry, I forgot to update violin.png after the latest changes to _anndata.py. Let's see if the tests pass now.

@WeilerP
Copy link
Contributor Author

WeilerP commented Sep 22, 2020

The tests have a tolerance parameter that is set high. The problem is that the stripplot shows different results each time. Also, different versions of matplotlib and seaborn have slight differences.

Ah yes, I see. The stripplot result could be fixed by setting a seed with np.random.seed. I doubt it will fix the difference due to the used version, though.

@fidelram fidelram merged commit 705a14e into scverse:master Sep 22, 2020
@dwnorton dwnorton mentioned this pull request Sep 22, 2020
Closed
@WeilerP WeilerP mentioned this pull request Nov 11, 2020
Closed
1 task
@hhhh1230511
Copy link

hhhh1230511 commented Nov 25, 2020

Hi Developers,

I got the same issue when using seaborn==0.11, and fortunately I got the issue solved by replacing the annotate.py with the modified version. Howerver, I was wondering why I tried to use 'pip install scanpy' to update the scripts, it failed? Is there any other easier method to modify the script, not to locate the file and replace it?

@WeilerP WeilerP deleted the fix/unit_tests_test_violin_and_test_pbmc3k branch November 26, 2020 19:48
@WeilerP
Copy link
Contributor Author

WeilerP commented Nov 26, 2020

@hhhh1230511, this PR is not part of any release yet (the latest version scanpy==1.6 was released August 15, 2020). If you want to have the latest version from GitHub you can follow the instructions for a developer installation here in the documentation, for example. Once a new release is available on pip, you can install it via pip install --upgrade scanpy
In general, you should avoid modifying the code by e.g. simply copying and pasting. This will either easily cause conflicts when updating the package or cause problems when functions from other files which depend on the content you changed but were not updated accordingly.

Hope this helped and clarified things.

@ivirshup ivirshup added the Maint – Backport needed Needs back porting for bugfix release label Dec 7, 2020
@ivirshup ivirshup removed the Maint – Backport needed Needs back porting for bugfix release label Jan 24, 2021
This was referenced Dec 14, 2021
Open
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Unit tests fail with latest version of seaborn

4 participants

@WeilerP @fidelram @hhhh1230511 @ivirshup