-
Notifications
You must be signed in to change notification settings - Fork 113
Add ViolinPlot Option to BoxPlot Operator #3356
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
Conversation
|
Please add the dataset used for testing in the description of this PR. |
GspikeHalo
left a comment
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.
LGTM!
.../main/scala/edu/uci/ics/amber/operator/visualization/boxViolinPlot/BoxViolinPlotOpDesc.scala
Show resolved
Hide resolved
.../main/scala/edu/uci/ics/amber/operator/visualization/boxViolinPlot/BoxViolinPlotOpDesc.scala
Show resolved
Hide resolved
.../main/scala/edu/uci/ics/amber/operator/visualization/boxViolinPlot/BoxViolinPlotOpDesc.scala
Show resolved
Hide resolved
.../main/scala/edu/uci/ics/amber/operator/visualization/boxViolinPlot/BoxViolinPlotOpDesc.scala
Show resolved
Hide resolved
Xiao-zhen-Liu
left a comment
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 left a few more additional code comments. Please create anther PR to address them.
In addition, here are a few other comments on the PR title and description:
- The PR "adds Violin Plot Operator support to the existing Box Plot Operator." Since you are not creating a new operator, please change the PR title to something like "Add ViolinPlot Option to BoxPlot Operator".
- PR descriptions should be treated like formal documentations and should not have grammar issues. In your "Changes" section of the PR description, make sure your bullet points have consistent tense and format. For example, "Updated createPlotlyFigure() to support both plot types." and "Update the user-friendly operator name (Box Plot -> Box/Violin Plot)" differ both in tense and type (one has a comma while the other does not).
…lot operator (#3365) 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 <img width="1439" alt="image" src="https://github.com/user-attachments/assets/b8e96412-aadb-4b96-97fb-16eb6a67179b" />


Purpose:
Changes:
In core/workflow-operator/src/main/scala/edu/uci/ics/amber/operator/visualization/boxViolinPlot/BoxViolinPlotOpDesc.scala:
violinplotwith a checkbox.violinplotis selected.In core/workflow-operator/src/main/scala/edu/uci/ics/amber/operator/LogicalOp.scala:
In core/gui/src/assets/operator_images/BoxViolinPlot.png:
Dataset for Test:
tips.csv