Boxplot element and category coordinate have been added. #83

nuald · 2019-11-15T16:24:59Z

Major changes:

Quartiles data object has been added (without outliers support as they are not used so much, but could consume a lot of memory and could have the performance penalty);
Boxplot element has been added (that draws the quartiles);
Category coordinate has been added (as boxplots are usually used for categorical views).

The corresponding example has been added (please note that it support the external data too as I've used it to verify how the dataset with few million rows is going to be calculated and presented). Unfortunately, changing the main README.md with an example image and its link would require a commit into the other repo, so it's out of scope of this PR.

Minor changes:

TextAlignment enum has been added as a hint for the rendering engines (used for SVG so far). SVG is rendered on the client side, so the layout based on fonts provided by fontkit is not always correct as the client side may use different fonts. The proper fix could be changing the signature of draw_text to use text style and rectangle, so the renderer would calculate the layout by itself, but it's rather a big change that could affect the performance (actually it may improve performance as certain engines don't require pre-calculation of a text block layout).
Clippy errors and warnings have been addressed or disabled (mostly to reduce the noise as I've used clippy for my changes). While some disabled warnings could be properly addressed, that could have the performance penalty and would require careful approach.
Testing has been added/improved for all the code changes I've introduced. As it's not quite trivial, I've added a notes in CONTRIBUTING.md for the future references.

Squashed commit of the following: Added testing notes. Added the test coverage for the changed code. Added documentation. TextAlignment enum has been added to be used as a hint for the rendering. Category coord has been added. Added explicit Quartiles struct. Initial Boxplot element has been added.

codecov · 2019-11-15T16:52:47Z

Codecov Report

Merging #83 into master will increase coverage by 3.44%.
The diff coverage is 84.69%.

@@            Coverage Diff             @@
##           master      #83      +/-   ##
==========================================
+ Coverage   52.39%   55.84%   +3.44%     
==========================================
  Files          47       50       +3     
  Lines        4321     4570     +249     
==========================================
+ Hits         2264     2552     +288     
+ Misses       2057     2018      -39

Impacted Files	Coverage Δ
src/coord/datetime.rs	`80.7% <ø> (ø)`	⬆️
src/style/color.rs	`38.88% <ø> (ø)`	⬆️
src/style/font/font_desc.rs	`42.85% <ø> (+4.39%)`	⬆️
src/element/candlestick.rs	`0% <ø> (ø)`	⬆️
src/coord/mod.rs	`15.38% <ø> (ø)`	⬆️
src/drawing/backend_impl/canvas.rs	`0% <0%> (ø)`	⬆️
src/drawing/backend_impl/cairo.rs	`0% <0%> (ø)`	⬆️
src/drawing/backend.rs	`32.81% <0%> (-1.06%)`	⬇️
src/drawing/area.rs	`93.47% <100%> (-0.08%)`	⬇️
src/drawing/backend_impl/mocked.rs	`91.08% <100%> (+0.18%)`	⬆️
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 216c1ce...c499bbd. Read the comment docs.

38 · 2019-11-15T20:00:47Z

Thanks for the contribution, this is a pretty concrete one!
I really like all of the things added and just look at the code roughly it looks pretty good.

Few more questions:

For the TextAlignment

This looks awesome, but a little bit confusing to me when I look at this first time.
Correct me if I am wrong.
If the size of the text is correctly estimated, there's no effect on the text positioning. (So bitmap backend can simple discard it)
My understand is this provides a reliable way that make sure even text size isn't correctly estimated, we still can make the our anchor point at the right location?
I really like this one, but if this is the case, we should have a better documentation.

For category coordinate

I think it's really good, I am thinking about this for a long time. It seems some of your code in this part: 1. get size of the category, 2. convert index to value 3. convert value to index, is used to describe some discrete coord spec and this is exactly what I am going to do when refactoring the discrete trait. By then, I think this coord can be simplified tremendously.
So I am a little bit hesitating on this one - SInce the discrete coordinate is to be refactored anyway, after that do we still need keep this or just like have an API issue #82 suggests. That may be a breaking change then if we remove the category coordinate.
So do you mind I keep this in version 0.2 and later when version 0.3 is published I would like to use the new discrete trait to replace this one? Thoughts ?

examples/boxplot.rs

src/drawing/backend_impl/svg.rs

src/chart/context.rs

nuald · 2019-11-15T20:52:04Z

So I am a little bit hesitating on this one - SInce the discrete coordinate is to be refactored anyway, after that do we still need keep this or just like have an API issue #82 suggests. That may be a breaking change then if we remove the category coordinate.
So do you mind I keep this in version 0.2 and later when version 0.3 is published I would like to use the new discrete trait to replace this one? Thoughts ?

It's completely up to you, I just needed to somehow present the labels and that's the only way I could do it. I'm sure you can do it much better :) I can even remove it from this PR and just left it in my own branch (as I need it right away, and thankfully Cargo can refer to github branches).

38 · 2019-11-16T00:13:44Z

It's completely up to you, I just needed to somehow present the labels and that's the only way I could do it. I'm sure you can do it much better :) I can even remove it from this PR and just left it in my own branch (as I need it right away, and thankfully Cargo can refer to github branches).

I think I just decided to keep it and replace it when 0.3 released.

So that's looks fine to me for now :)

- the boxplot example have been fixed to draw the series in a sorted order and use `draw_series` API rather than direct calls to area API; - area `titled` use center alignment now; - TextAlignment description have been clarified to mention invariant points.

38

it looks good to me, just have some minor thing.
One thing I feel the most important thing is we need to document TextAlignment a little bit more, since the purpose for this is not obvious to everybody.

It's up to you to make the change, since it's all minor thing. If you don't want to change it, I can resolve them it before I merge it.

Thanks again for this great work! Cheers!

src/coord/category.rs

src/data/quartiles.rs

src/style/text.rs

src/drawing/backend_impl/bitmap.rs

38

It looks good, but we can do further simplification on the Category part. I have tried that and I just want to make a PR on your PR that makes category simplier. After that I think we are good to go.
Thanks!

38 · 2019-11-18T17:55:46Z

src/coord/category.rs

-/// The category elements range.
-pub struct CategoryElementsRange<T: PartialEq>(CategoryElementRef<T>, CategoryElementRef<T>);
+/// The category elements range
+pub struct CategoryElementsRange<T: PartialEq>(Category<T>, Category<T>);


I am thinking about this, as long as we only using all values in a Category as an range. It seems this range also can be merged into Category type.
It's just minor change, I'd like to make a PR to you to show what I mean :)

Thank you, I'd appreciate it!

38 · 2019-11-19T01:21:18Z

Just Merged the PR. Thanks for the contribution. Really appreciated for this concrete work. :)

wangjiawen2013 · 2024-02-21T05:27:52Z

@nuald
Hi,
I have tested boxplot, it looks good except a little flaw:

nuald mentioned this pull request Nov 15, 2019

[Feature Request] Slice as a discrete coordinate #82

Closed

38 reviewed Nov 15, 2019

View reviewed changes

examples/boxplot.rs Outdated Show resolved Hide resolved

examples/boxplot.rs Show resolved Hide resolved

src/drawing/backend_impl/svg.rs Show resolved Hide resolved

src/chart/context.rs Show resolved Hide resolved

38 added the good first issue Good for newcomers label Nov 16, 2019

nuald added 2 commits November 16, 2019 10:05

Merge branch 'master' into s-boxplot

9404d03

nuald requested a review from 38 November 16, 2019 17:11

38 approved these changes Nov 16, 2019

View reviewed changes

src/coord/category.rs Outdated Show resolved Hide resolved

src/data/quartiles.rs Show resolved Hide resolved

src/style/text.rs Outdated Show resolved Hide resolved

38 reviewed Nov 17, 2019

View reviewed changes

src/drawing/backend_impl/bitmap.rs Outdated Show resolved Hide resolved

nuald added 2 commits November 18, 2019 08:16

Merge branch 'master' into s-boxplot

0ea7434

Simplified Category coord impl and improved Quartiles calculations.

c499bbd

nuald requested a review from 38 November 18, 2019 17:45

38 requested changes Nov 18, 2019

View reviewed changes

38 merged commit c499bbd into plotters-rs:master Nov 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Boxplot element and category coordinate have been added. #83

Boxplot element and category coordinate have been added. #83

nuald commented Nov 15, 2019

codecov bot commented Nov 15, 2019 •

edited

Loading

38 commented Nov 15, 2019 •

edited

Loading

nuald commented Nov 15, 2019

38 commented Nov 16, 2019

38 left a comment

38 left a comment

38 Nov 18, 2019

nuald Nov 18, 2019

38 commented Nov 19, 2019

wangjiawen2013 commented Feb 21, 2024

Boxplot element and category coordinate have been added. #83

Boxplot element and category coordinate have been added. #83

Conversation

nuald commented Nov 15, 2019

codecov bot commented Nov 15, 2019 • edited Loading

Codecov Report

38 commented Nov 15, 2019 • edited Loading

nuald commented Nov 15, 2019

38 commented Nov 16, 2019

38 left a comment

Choose a reason for hiding this comment

38 left a comment

Choose a reason for hiding this comment

38 Nov 18, 2019

Choose a reason for hiding this comment

nuald Nov 18, 2019

Choose a reason for hiding this comment

38 commented Nov 19, 2019

wangjiawen2013 commented Feb 21, 2024

codecov bot commented Nov 15, 2019 •

edited

Loading

38 commented Nov 15, 2019 •

edited

Loading