Bar charts + box plots continued #863

Bromeon · 2021-11-01T13:48:54Z

The addition of box/bar charts in #499 looks very valuable to me. Unfortunately the original PR seems to have been abandoned and has caused several merge conflicts in the meantime. This PR adds commits on top to update to egui's latest API. In case #499 is continued, I'm happy to close this and let my changes be integrated there 🙂

Now there are some open topics:

I didn't touch the existing commits to retain the author information; might be worth a squash. In case we rebase rather than merge latest master into the branch (as done now), we would probably require @niladic's and my changes to be combined into a single commit (if that's the case, Co-authored-by could help).
The "closest" algorithm is not yet integrated, as there wasn't yet any consensus on how to do it efficiently, so debug information for hovering over charts won't show. Also, is the idea that every plot type implements that algorithm on their own? Why not return something like an "outline shape" and compute close-ness based on that?
I wasn't sure about the initialize() methods. The trivial implementation would be the following, but is this meaningful?
```
fn initialize(&mut self, x_range: RangeInclusive<f64>) {
    self.dummy_values.generate_points(x_range);
}
```
Naming: the original PR uses very mixed BarChart, barchart, Boxplot, boxplots symbols. I would probably change this to use PascalCase/snake_case consistently.
Generally, I'm quite new to egui, so let me know where things can be improved!

emilk · 2021-11-01T19:08:21Z

@EmbersArc what do you think?

EmbersArc

Thanks so much for reviving this PR!

I'm still not sure about the ergonomics of the stacked bar charts, but I'd have to use it to be able to tell really. I think once the dead code (e.g. rulers) is either removed or used in some way, we can merge this without issues.

EmbersArc · 2021-11-02T21:17:30Z

egui/src/widgets/plot/transform.rs

@@ -8,6 +8,7 @@ use crate::*;
 #[derive(Clone, Copy, PartialEq, Debug)]
 #[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))]
 pub(crate) struct Bounds {
+    /// [x, y] array


Suggested change

/// [x, y] array

EmbersArc · 2021-11-03T22:00:10Z

I wasn't sure about the initialize() methods. The trivial implementation would be the following, but is this meaningful?

No it's not. You can just leave this function empty. We can try to think about how to extend the PlotItem trait later so that we can make it public and users can create their own.

Bromeon · 2021-11-04T16:40:46Z

@EmbersArc the previous PR could highlight the closest bars on hovering, as well as display some associated information:

This is currently disabled, but I could find a way to re-enable it, as it seems very useful.
My question is, if there is already a recommended way/pattern to achieve this? Or should I experiment a bit?

Bromeon · 2021-11-04T16:48:41Z

Also, any advice regarding the previous commits? Should I squash them, use Co-authored-by, or something else?

EmbersArc · 2021-11-04T19:28:33Z

This is currently disabled, but I could find a way to re-enable it, as it seems very useful.

If I remember correctly, the previous PR would have added something like an on_hover_shapes to the PlotItem trait. The basic idea was that this would take the mouse coordinate and then return a vector of shapes that would show more information about the hovered item. I think this wasn't a bad design, so you could try adding that again. But feel free to experiment :)

Signed-off-by: Jan Haller <bromeon@gmail.com>

# Conflicts: # CHANGELOG.md # egui/src/widgets/plot/items.rs # egui/src/widgets/plot/mod.rs # egui_demo_lib/src/apps/demo/plot_demo.rs

Bromeon · 2021-11-14T22:59:53Z

I managed to find some time to overhaul the existing implementation and clean up a lot. Bar charts + box plots are now working, and the hovering logic is running as well.

Unfortunately, during exactly that time #766 has landed, and #892 is on the way too, both could create significant merge conflicts.
Let me know what would be a good strategy here.

My implementation is not perfect yet, but I'd rather create a follow-up PR than diverging more and risking even more merge conflicts.

Change box plot/bar chart examples to new PlotUi API # Conflicts: # egui/src/widgets/plot/mod.rs

Rename: * BoxplotDiagram -> BoxPlot * Boxplot -> BoxElem * boxplots() -> box_plot()

Bromeon · 2021-11-15T13:52:13Z

@EmbersArc Fixed the merge conflicts with current master branch.
This pull request should be ready for review now. ✔️

Worth mentioning: apart from the plots, I added the (private) FloatOrd trait, which allows to use my_value.ord() to quickly create a f32 value implementing Ord. This is needed for algorithms like sorted(), min_by_key() etc. Maybe others can benefit from this as well, but let me know if there's already an established way (or dependency) to achieve that.

I would like to express huge thanks to @niladic, who laid out the foundation and much of the work in his original PR. I made sure to attribute him as author of the original commit. 👍

s-nie

wrong account again, please ignore

EmbersArc

Gave it a quick look. Looks very good! Just a couple smaller comments.

egui/src/widgets/plot/items.rs

egui/src/widgets/plot/mod.rs

Bromeon · 2021-11-17T16:47:26Z

Thanks for the feedback!

Implemented two suggestions; only the ord() topic left open until clarified.

emilk

Looks really good to me! I agree there should be a demo of the "ruler" code.

egui/src/util/float_ord.rs

emilk · 2021-11-27T11:21:33Z

egui/src/util/float_ord.rs

@@ -0,0 +1,40 @@
+//! Total order on floating point types, assuming absence of NaN.
+//! Can be used for sorting, min/max computation, and other collection algorithms.


I like this idea of having our own OrderedFloat without having to pull in the much bigger ordered-float crate. There already is an epaint::f32_hash function in epaint/src/lib.rs. Perhaps we should implement Hash on OrderedFloat and move it to epaint/src/util/ordered_float.rs.

But let's make it support NaN instead of panicing on them!

I can also help with this refactor after merge

Thanks! I'll gladly open a new PR after this one, but probably makes sense to keep it out of this (already big) changeset.

egui/src/util/float_ord.rs

egui/src/widgets/plot/items.rs

egui/src/widgets/plot/mod.rs

egui/src/widgets/plot/transform.rs

emath/src/pos2.rs

Bromeon

Thanks for the feedback!

Not yet all points addressed, will continue tomorrow.

egui/src/widgets/plot/transform.rs

emath/src/pos2.rs

Bromeon · 2021-11-27T16:35:12Z

egui/src/util/float_ord.rs

@@ -0,0 +1,40 @@
+//! Total order on floating point types, assuming absence of NaN.
+//! Can be used for sorting, min/max computation, and other collection algorithms.


Thanks! I'll gladly open a new PR after this one, but probably makes sense to keep it out of this (already big) changeset.

Changes (non-exhaustive): * FloatOrd supports NaN * BoxElem combines 5 values in BoxSpread struct * Split bar/box-related functionality into multiple files * Pos2::distance_from_rect_sq() -> Rect::distance_sq_to_pos() * Consistent terminology (key -> argument)

# Conflicts: # egui/src/widgets/plot/items.rs # egui/src/widgets/plot/mod.rs # egui/src/widgets/plot/transform.rs

Bromeon · 2021-11-28T22:01:45Z

Merged latest master (some conflicts due to Bounds -> PlotBounds etc. in #892) and implemented requested changes.

@emilk As you noted correctly, the file items.rs has grown quite a bit. I turned it into a module folder, with several files for bars/boxes. I also outsourced all the "helper" types which are needed for plots and values -- but not directly related to PlotItem -- to their own module values. Now, items contains mostly the different plot types and their respective impl PlotItem.

I agree there should be a demo of the "ruler" code.

What do you mean exactly? Also, if it's a feature, would it be possible to do it in another PR? I've just resolved the third big merge conflict due to branch divergence, and with the new file structure there's quite some risk for nasty conflicts 😀

Edit: I'm not sure about the merge policy, but it looks like you typically squash the commits to a single one. In case you do that, could you use the below commit message? That way, not only I receive credits, but the original author (niladic) as well.

This commit message should be ready to copy/paste:

Add bar charts and box plots (#863)

Changes:
* New `BarChart` and `BoxPlot` diagrams
* New `FloatOrd` trait for total ordering of float types
* Refactoring of existing plot items

Co-authored-by: niladic <git@nil.choron.cc>

Changes: * Move implementation details of items under sub-modules in items * Outsource helper structs to 'values' module * Simplify stacking implementation, add f64::ord() impl * Rename HoverElem -> ClosestElem

Bromeon · 2021-11-29T19:21:30Z

Thanks a lot to both of you for review and feedback! It's good to have the initial implementation merged, there might be some API refinement once users get their hands on.

In the meantime I moved ahead with OrderedFloat in #918.

EmbersArc reviewed Nov 3, 2021

View reviewed changes

niladic and others added 3 commits November 8, 2021 22:43

Add bar charts and boxplots.

de33739

Signed-off-by: Jan Haller <bromeon@gmail.com>

Merge branch 'master' into feature/barcharts; adjust to latest API

fec3b8d

# Conflicts: # CHANGELOG.md # egui/src/widgets/plot/items.rs # egui/src/widgets/plot/mod.rs # egui_demo_lib/src/apps/demo/plot_demo.rs

Add util for total ordering of f32

865c6bf

Bromeon marked this pull request as ready for review November 14, 2021 22:56

Bromeon added 3 commits November 15, 2021 00:19

Boxplot/bar chart: clean up, document and remove code duplication

16a62c3

Merge branch 'master' into feature/barcharts

5ac60c6

Change box plot/bar chart examples to new PlotUi API # Conflicts: # egui/src/widgets/plot/mod.rs

Use correct terminology (a box plot is a diagram and not an element)

d045556

Rename: * BoxplotDiagram -> BoxPlot * Boxplot -> BoxElem * boxplots() -> box_plot()

s-nie suggested changes Nov 16, 2021

View reviewed changes

EmbersArc suggested changes Nov 16, 2021

View reviewed changes

egui/src/widgets/plot/items.rs Outdated Show resolved Hide resolved

egui/src/widgets/plot/mod.rs Show resolved Hide resolved

egui/src/widgets/plot/mod.rs Outdated Show resolved Hide resolved

emilk requested changes Nov 27, 2021

View reviewed changes

Bromeon commented Nov 27, 2021

View reviewed changes

emilk approved these changes Nov 28, 2021

View reviewed changes

Bromeon added 2 commits November 28, 2021 20:32

Merge branch 'master' into feature/barcharts

a7e85d0

# Conflicts: # egui/src/widgets/plot/items.rs # egui/src/widgets/plot/mod.rs # egui/src/widgets/plot/transform.rs

Reorganize plot::items file structure

f2e3149

Changes: * Move implementation details of items under sub-modules in items * Outsource helper structs to 'values' module * Simplify stacking implementation, add f64::ord() impl * Rename HoverElem -> ClosestElem

emilk merged commit 1088d95 into emilk:master Nov 29, 2021

emilk mentioned this pull request Nov 29, 2021

Add bar charts and boxplots #499

Closed

Bromeon deleted the feature/barcharts branch November 29, 2021 18:00

Bromeon mentioned this pull request Nov 29, 2021

OrderedFloat refactor #918

Merged

Bromeon mentioned this pull request Jan 16, 2022

More flexible plotting (histogram, box plot...) #467

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bar charts + box plots continued #863

Bar charts + box plots continued #863

Bromeon commented Nov 1, 2021

emilk commented Nov 1, 2021

EmbersArc left a comment

EmbersArc Nov 2, 2021

EmbersArc commented Nov 3, 2021

Bromeon commented Nov 4, 2021

Bromeon commented Nov 4, 2021

EmbersArc commented Nov 4, 2021

Bromeon commented Nov 14, 2021

Bromeon commented Nov 15, 2021

s-nie left a comment •

edited

Loading

EmbersArc left a comment

Bromeon commented Nov 17, 2021

emilk left a comment

emilk Nov 27, 2021

emilk Nov 27, 2021

Bromeon Nov 27, 2021

Bromeon left a comment

Bromeon Nov 27, 2021

Bromeon commented Nov 28, 2021 •

edited

Loading

Bromeon commented Nov 29, 2021

		@@ -0,0 +1,40 @@
		//! Total order on floating point types, assuming absence of NaN.
		//! Can be used for sorting, min/max computation, and other collection algorithms.

Bar charts + box plots continued #863

Bar charts + box plots continued #863

Conversation

Bromeon commented Nov 1, 2021

emilk commented Nov 1, 2021

EmbersArc left a comment

Choose a reason for hiding this comment

EmbersArc Nov 2, 2021

Choose a reason for hiding this comment

EmbersArc commented Nov 3, 2021

Bromeon commented Nov 4, 2021

Bromeon commented Nov 4, 2021

EmbersArc commented Nov 4, 2021

Bromeon commented Nov 14, 2021

Bromeon commented Nov 15, 2021

s-nie left a comment • edited Loading

Choose a reason for hiding this comment

EmbersArc left a comment

Choose a reason for hiding this comment

Bromeon commented Nov 17, 2021

emilk left a comment

Choose a reason for hiding this comment

emilk Nov 27, 2021

Choose a reason for hiding this comment

emilk Nov 27, 2021

Choose a reason for hiding this comment

Bromeon Nov 27, 2021

Choose a reason for hiding this comment

Bromeon left a comment

Choose a reason for hiding this comment

Bromeon Nov 27, 2021

Choose a reason for hiding this comment

Bromeon commented Nov 28, 2021 • edited Loading

Bromeon commented Nov 29, 2021

s-nie left a comment •

edited

Loading

Bromeon commented Nov 28, 2021 •

edited

Loading