-
Notifications
You must be signed in to change notification settings - Fork 612
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
(fix): resolve data ordering to match axis for stacked violin plots #3196
Conversation
ilan-gold
commented
Aug 5, 2024
- Closes Incorrect column label and color assign when using 'rank_genes_groups_stacked_violin' with 'swap_axes=True' #3152
- Tests included or not required because:
- Release notes not necessary because:
Will finish after lunch....wowza |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3196 +/- ##
==========================================
+ Coverage 76.61% 76.63% +0.01%
==========================================
Files 109 109
Lines 12532 12533 +1
==========================================
+ Hits 9602 9605 +3
+ Misses 2930 2928 -2
|
Not exactly sure how to test this - it's not that the axis is misordered, it's that we were not informing the violin plot of this ordering. I am not sure if there is a way to access the underlying data of a plot... |
I have some limited experience with that plot order stuff. There’s some compat code for old pandas versions added in #2816 which shows some spots where we mess with ordering. Generally
|
@flying-sheep I think the code added in that PR is minimally relevant to what happened here.
Right, so here the issue is that the category ordering is used for the labelling but we were not imposing it on the data itself when the violin plots render (separate from the axis labels, as the actual violin plots are added row-by-row).
In some sense the above also applies. If we want to add some sort of user-facing part of the API to allow for ordering, that is fine, but I think that should be separate as it would go into the next minor release and this is a fairly large bug. I'm fine not testing this because I genuinely don't know how and I spent a few hours yesterday trying different things to no avail. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I trust you that this helps 🤷
I guess we can add a test that cements this behavior, but it's tough to test that swap=original-transpose for the underlying data. I tried applying the ordering to what changed in the PR you referenced but that didn't help unfortunately. I don't know the provenance of that change, so I think this is fair. We are using the public API of seaborn in a way that fixes the underlying issue as one would expect. |
…for stacked violin plots
…atch axis for stacked violin plots) (#3200) Co-authored-by: Ilan Gold <[email protected]>
…cverse#3196) * (fix): resolve axis ordering * (chore): release note * (chore): add test * (fix): make testing faster and more robust * (fix): light refactor * (fix): clean up comments * (chore): add more comments * (fix): remove test * Update src/scanpy/plotting/_stacked_violin.py * (chore): add test to cement behavior * (fix): tolerance that is sensitive enough for `main` difference --------- Co-authored-by: Philipp A. <[email protected]>