-
Notifications
You must be signed in to change notification settings - Fork 99
Pareto chart implementation #431
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
Co-authored-by: John Casagranda <john.casagranda@gmail.com> Co-authored-by: Gauthier Segay <gauthier.segay@gmail.com>
Co-authored-by: John Casagranda <john.casagranda@gmail.com> Co-authored-by: Gauthier Segay <gauthier.segay@gmail.com>
Co-authored-by: John Casagranda <john.casagranda@gmail.com> Co-authored-by: Gauthier Segay <gauthier.segay@gmail.com>
Co-authored-by: John Casagranda <john.casagranda@gmail.com> Co-authored-by: Gauthier Segay <gauthier.segay@gmail.com>
@kMutagene if you have an idea how to adjust so that the legend at the top right doesn't overlay on top of the "100%" label, please let me know. I'm wondering also if we should have optional parameter that would take the separate plots to alter them: type ParetoChartPart =
| ColumnPart
| ParetoLinePart
Chart.Pareto(...
, customizeChartPart=[
ColumnPart, (fun chart -> chart |> Chart.with...)
]
) This technique could be used in the different composed charts. |
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.
Hi guys, this looks great. This is in general ready to merge. if you want to improve a bit you can use a single line chart instead of one point and one line chart, and expose more customizability via optional arguments. Otherwise, LGTM!
src/Plotly.NET/ChartAPI/Chart2D.fs
Outdated
|
||
let bars = Chart.Column(Seq.zip orderedLabels orderedValues,?Name=Name) | ||
|
||
let points = |
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.
there is no need for 2 traces for points and lines here, you can show markers on a line chart with ShowMarkers = true
.
|> Chart.withYAxisStyle ( | ||
?TitleText = Label | ||
, Id = StyleParam.SubPlotId.YAxis 1 | ||
, ShowGrid = false |
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 usually expose settings like this as method argument, and setting sensible default values.
Also, a version using labels and values as separate sequences instead of tupled values would be nice as well, but not necessary. This would have the advantage that we do not need a custom tuple type for the C# version, and instead expose the method using separate sequences (this is what i did with all other C# methods) |
@kMutagene thanks for the feedback, we'll go over the suggestions / adjustments probably over Sunday. |
…hart instead, adding overloads to take the labels and values separately, adding ShowGrid as an option. Co-authored-by: John Casagranda <john.casagranda@gmail.com> Co-authored-by: Gauthier Segay <gauthier.segay@gmail.com>
@kMutagene I made adjustments, the main remarks I have:
Let see if people need specific customizations. |
Can you give an example of mismatched data? I think i do not understand exactly.
Can also be prevented by styling the markers of the line chart accordingly, but i think this can be done after merging this PR. In general, this is a really high quality PR as it even includes documentation, thanks @smoothdeveloper and @rockfaith75 ! The only thing needed for release is addition of some tests, but since it is quite complicated to set up tests in this repo I will take care of that. Thanks again! |
like let values = [1.;2.;3.]
let labels = ["a";"b"] // missing one label
Chart2D.Pareto(labels, values) Thanks for the review and we'll be on the lookout for how the tests are added for future contributions. |
I see, but this is handled by plotly.js by simply only displaying data that has both dimensions, so we overall do not really care about this across the lib. |
@kMutagene, this is for #423, please let us know what changes you want.
Here is snippet of rendered documentation:
cc: @rockfaith75