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

Violin improvements #2875

Merged
merged 7 commits into from
Feb 25, 2025
Merged

Violin improvements #2875

merged 7 commits into from
Feb 25, 2025

Conversation

airenzp
Copy link
Collaborator

@airenzp airenzp commented Feb 25, 2025

Description

This PR fixes #2839 by reordering the rendering logic: First render a plot, then get its size and based on that size position the next plot. It also does some cleanup in the code and adds a choice to configure the plot padding.

Checklist

Check each task that has been performed or verified to be not applicable.

  • Tests: added and/or passed unit and integration tests, or N/A
  • Todos: commented or documented, or N/A
  • Notable Changes: updated release.txt, prefixed a commit message with "fix:" or "feat:", added to an internal tracking document, or N/A

@airenzp airenzp requested review from xzhou82 and gavrielm February 25, 2025 16:59
@gavrielm
Copy link
Collaborator

Thanks looks nice. It seems like violin plots of some variables, for example heart radiation and alkylating agents, look shorter than violin plots of other variables, for example age and blood pressure. Do you see the same thing? The plot thickness should be the same, right?

@airenzp
Copy link
Collaborator Author

airenzp commented Feb 25, 2025

It is because of the smoothing applied on the client to the path being rendered, this is done after building the plot, it causes that the plot size is not the same for all the plots. The heart plots has some peaks that are more smoothed. I hope that this makes sense. I also added a padding parameter. We could make it smaller by default if you still feel is a lot of padding. But we have to check it looks ok for all the plots. By default is 5.

@gavrielm
Copy link
Collaborator

I see. Padding is value is fine, so no need to change that. The heart radiation and alkylating agents plots do look a bit small by default, so it may be good to increase the default plot thickness. Im not too tied to it though, so if @xzhou82 is fine with them then I can approve.

@airenzp
Copy link
Collaborator Author

airenzp commented Feb 25, 2025

I updated the default size. I think is a good idea to increase it as that is the most common case and when there are many categories is ok to scroll. Please check.

@airenzp
Copy link
Collaborator Author

airenzp commented Feb 25, 2025

I also updated the SC plot thickness as the plots looked so small:

Screenshot 2025-02-25 at 1 31 29 PM

Aferwards:

Screenshot 2025-02-25 at 1 33 43 PM

@gavrielm
Copy link
Collaborator

Great thanks. The increased plot thickness looks better.

Seems like spacing between categories is now too small (example):
image

Can spacing be increased?

@airenzp
Copy link
Collaborator Author

airenzp commented Feb 25, 2025

Yes I suppose can you update it? I am in another branch.

gavrielm
gavrielm previously approved these changes Feb 25, 2025
Copy link
Collaborator

@gavrielm gavrielm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I increased plot padding to 10 and slightly decreased plot thickness to 130. If you think looks ok then we can merge

@airenzp
Copy link
Collaborator Author

airenzp commented Feb 25, 2025

I guess we can wait, this is not a breaking change

@@ -616,7 +616,7 @@ class singleCellPlot {
plots: [
{
chartType: 'violin',
settings: { violin: { plotThickness: 50 } },
settings: { violin: { plotThickness: 90 } },
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

90 too tall for seurat clusters which are usually many. i would like to revert to 50
also this really should be auto set based on number of categories

@airenzp
Copy link
Collaborator Author

airenzp commented Feb 25, 2025

I pushed a fix to calculate the plots size based on the number of plots unless a plotThickness is specified. Please check.

Copy link
Collaborator

@xzhou82 xzhou82 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

much better. thanks

@xzhou82 xzhou82 merged commit 1a612f6 into master Feb 25, 2025
3 checks passed
@xzhou82 xzhou82 deleted the issue.2839 branch February 25, 2025 22:09
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.

Large space between plot and axis in violin plot
3 participants