Skip to content
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

add raincloud plots from Beacon Biosignals #1725

Merged
merged 22 commits into from Jun 3, 2022
Merged

Conversation

SimonDanisch
Copy link
Member

We decided to open source @kimlaberinto amazing raincloud plots and add them to base Makie.
This is a direct port of the docs & the latest implementation.
@kimlaberinto wanted to spent some more time to make the recipe even more usable/pretty before we merge.
We also need to add some tests to the reference image tests.

cc: @haberdashPI @ericphanson @mich11 @palday @jrevels

@kimlaberinto
Copy link
Contributor

@haberdashPI mentioned this in a previous discussion that we've had. I wanted to repost it here so others can see and so that we potentially make this change as well.

I want to advocate for the API that is made public to be compatible with AlgebraOfGraphics. I think it's a simple change and it would be nice if it worked straightforwardly in that case: basically the interface needs to use a flat array (like columns of a DataFrame) to denote the category labels along x, rather than using nested arrays by default. I was thinking of making a little PR myself to do this...

@MakieBot
Copy link
Collaborator

MakieBot commented Mar 6, 2022

Compile Times benchmark

Note, that these numbers may fluctuate on the CI servers, so take them with a grain of salt.

using time

master:  9.11 < 9.12 > 9.18, 0.02+-
pr:      9.10 < 9.14 > 9.24, 0.04+-
speedup: 0.99 < 1.00 > 1.00, 0.01+-
median:  +0.24% => invariant

This PR does not change the using time.

ttfp time

master   25.53 < 25.61 > 25.73, 0.07+-
pr       25.47 < 25.57 > 25.91, 0.14+-
speedup: 0.99 < 1.00 > 1.01, 0.01+-
median:  -0.16% => invariant

This PR does not change the ttfp time.

@haberdashPI
Copy link
Contributor

haberdashPI commented Apr 25, 2022

I've added some proposed changes in #1839. See that PR for details. I've tested this with a PR of AoG which supports the latest version of Makie (by just updated the [compat] entries), and that seems to be working well for calling visual(RainClouds).

haberdashPI and others added 2 commits May 2, 2022 18:54
* support flat array style for raincloud plot

* hacky fix

* revise category labels

* fix labels

* fix x labels again

* working dodge / color

* update docs

* update news/docs

* moved news item

* fix `rainclouds` news item
@haberdashPI
Copy link
Contributor

Sorry about that, it looks like my use of some newer keyword syntax is leading to the failures on 1.3: (;show_median) instead of (;show_median=show_median)

SimonDanisch and others added 2 commits May 3, 2022 19:54
Co-authored-by: David Little <david.frank.little@gmail.com>
Co-authored-by: David Little <david.frank.little@gmail.com>
NEWS.md Outdated Show resolved Hide resolved
@SimonDanisch
Copy link
Member Author

Ok, this should build now...
I removed dodge here: ba50702, since group_labels didn't seem to have the ability to deal with dodge...Not sure if zipping dodge in there was just an oversight, or if group_labels should be able to deal with that.
Also, it's all gray now, I think before, it was cycling the color?

@haberdashPI
Copy link
Contributor

haberdashPI commented May 4, 2022

It should probably be cycling color; I can investigate/debug today or tomorrow. Is there anything special I should know to setup the doc build on my machine?

@SimonDanisch
Copy link
Member Author

add raincloud plots from Beacon Biosignals by SimonDanisch · Pull Request #1725 · JuliaPlots/Makie.jl

I think it was doing that before, if I'm not mistaken.. So think something got removed ;)

Is there anything special I should know to setup the doc build on my machine?

I usually just copy out the code blocks, since the setup is a bit annoying and you can't just build a single page 😅

@haberdashPI haberdashPI mentioned this pull request May 13, 2022
7 tasks
@haberdashPI
Copy link
Contributor

I've made some updates to the docs in #1936 .

This also fixes a few bugs I found in the process.

haberdashPI and others added 2 commits May 13, 2022 16:43
* improve raincloud examples

* improve docs

* fix hist coloring

* remove redundant computations
@SimonDanisch
Copy link
Member Author

Test failures due to: #1937

NEWS.md Outdated Show resolved Hide resolved
####

function mockup_distribution(N)
all_possible_labels = ["Single Mode", "Double Mode", "Single Mode", "Double Mode",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it intentional that single mode and double mode show up twice here?

Comment on lines +28 to +31
random_mean = rand_localized(0, 8)
random_spread_coef = rand_localized(0.3, 1)
data_points = random_spread_coef*randn(Int(round(N/2.0))) .+ random_mean

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
random_mean = rand_localized(0, 8)
random_spread_coef = rand_localized(0.3, 1)
data_points = random_spread_coef*randn(Int(round(N/2.0))) .+ random_mean

potential duplication? otherwise these variables get overridden immediately

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks right to me. (Note the vcat on line 33).

@haberdashPI
Copy link
Contributor

haberdashPI commented Jun 2, 2022

What's left to have this merge? I thought small docs fixes addressed @hannahilea's comments based on the name, but it looks like that's actually fixing something else. Happy to address those unresolved issues in a new PR to merge here.

@SimonDanisch
Copy link
Member Author

Yes, I think that's what's mostly left.
Would also be nice to bring back the color cycling. Let me know if you need help with that.

@haberdashPI
Copy link
Contributor

Would also be nice to bring back the color cycling. Let me know if you need help with that.

I think this is addressed? At least when I ran the help examples on my machine they are colored now. (#1936)

I'll set up a PR to fix @hannahilea's comments.

@SimonDanisch
Copy link
Member Author

Ah you're right, great :) Somehow the last time I checked the docs build, it was still all black

@haberdashPI
Copy link
Contributor

#2019 makes a small fix to address @hannahilea's comments.

haberdashPI and others added 2 commits June 3, 2022 17:47
* small fix to raincloud docs

* fix modes
@SimonDanisch SimonDanisch merged commit 0022960 into master Jun 3, 2022
@SimonDanisch SimonDanisch deleted the sd/rainclouds branch June 3, 2022 16:38
@SimonDanisch
Copy link
Member Author

Yay, thanks everyone :)

@@ -249,7 +249,7 @@ function plot!(plot::RainClouds)
show_median=show_median, side=side, width=width_ratio*cloud_width, plot.cycle,
plot.color, gap=0)
elseif clouds === hist
for (_, ixs) in group_labels(zip(category_labels, plot.dodge[]), data_array)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just realize that this was actually intentional. Without this, the histogram doesn't group by dodge, and so you get one histogram for all of the dodge groups. 😢

I'm submitting a small PR soon to implement orientation=:horizontal for rainclouds. I'll add a fix for this in that PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants