Skip to content

Conversation

@qzt168
Copy link
Contributor

@qzt168 qzt168 commented Apr 10, 2025

This PR fixes a previous PR (#3356), and includes the following updates:

  • Renamed variables for better clarity and consistency:
    • orientation → horizontalOrientation
    • violinplot → violinPlot
    • quartiletype → quartileType
  • Updated the Violin Plot checkbox description to clarify its behavior
  • Revised the operator description to explain that the violin plot overlays the box plot for better visualization
  • Modified the createPlotlyFigure() method:
    • Now supports quartileType when box=True in a violin plot
    • Removed jitter=0 to allow data points to spread naturally instead of forming a straight line — this shows the distribution shape better
image

@qzt168 qzt168 added the fix label Apr 10, 2025
@qzt168 qzt168 self-assigned this Apr 10, 2025
Copy link
Contributor

@Xiao-zhen-Liu Xiao-zhen-Liu left a comment

Choose a reason for hiding this comment

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

This PR makes your implementation look much better. Thanks!

Another general comment: It is a common practice within our team to let title of a PR be a complete sentence that usually starts with a verb. Please fix the title of this PR accordingly.

@qzt168 qzt168 changed the title Fixes for BoxViolinPlot Operator (Descriptions, Variables, Functions) Fix the descriptions, variable names, and functions in the BoxViolinPlot operator Apr 11, 2025
Copy link
Contributor

@GspikeHalo GspikeHalo left a comment

Choose a reason for hiding this comment

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

Thanks for the update! Perhaps you could move the required fields like value and quartileType to the top of the parameter list, making it more intuitive.

@qzt168 qzt168 requested a review from GspikeHalo April 12, 2025 00:23
Copy link
Contributor

@GspikeHalo GspikeHalo left a comment

Choose a reason for hiding this comment

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

LGTM!

@qzt168 qzt168 merged commit 10fced5 into master Apr 12, 2025
8 checks passed
@qzt168 qzt168 deleted the qizhi-fix-violinplot-operator branch April 12, 2025 03:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants