-
Notifications
You must be signed in to change notification settings - Fork 113
Fix the descriptions, variable names, and functions in the BoxViolinPlot operator #3365
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
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.
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.
.../main/scala/edu/uci/ics/amber/operator/visualization/boxViolinPlot/BoxViolinPlotOpDesc.scala
Outdated
Show resolved
Hide resolved
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.
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.
.../main/scala/edu/uci/ics/amber/operator/visualization/boxViolinPlot/BoxViolinPlotOpDesc.scala
Outdated
Show resolved
Hide resolved
.../main/scala/edu/uci/ics/amber/operator/visualization/boxViolinPlot/BoxViolinPlotOpDesc.scala
Outdated
Show resolved
Hide resolved
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!
This PR fixes a previous PR (#3356), and includes the following updates:
quartileTypewhenbox=Truein a violin plotjitter=0to allow data points to spread naturally instead of forming a straight line — this shows the distribution shape better