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

ppc_violin_grouped: plot y as violin, too #74

Merged
merged 4 commits into from Mar 13, 2017

Conversation

silberzwiebel
Copy link
Contributor

I plotted a reaction time analysis using ppc_violin_grouped and got this:

pointdata

However, I personally find this plot more informative as it shows the density of the empirical data also:

violindata

I created a pull request to deliver the solution to my problem instead of just opening an issue and complaining. Possibly, however, there are reasons out of my knowledge why the data is plotted as geom_point and not geom_violin.
I'm also not sure about any possible conventions regarding the plotting style of data. I found the current style easy to see for my example.

@codecov-io
Copy link

codecov-io commented Feb 28, 2017

Codecov Report

Merging #74 into master will decrease coverage by 0.3%.
The diff coverage is 74.07%.

@@            Coverage Diff             @@
##           master      #74      +/-   ##
==========================================
- Coverage     100%   99.69%   -0.31%     
==========================================
  Files          28       28              
  Lines        2918     3274     +356     
==========================================
+ Hits         2918     3264     +346     
- Misses          0       10      +10
Impacted Files Coverage Δ
R/ppc-distributions.R 95.65% <74.07%> (-4.35%)
R/mcmc-alg-nuts.R 100% <0%> (ø)
R/mcmc-diagnostics.R 100% <0%> (ø)
R/bayesplot-helpers.R 100% <0%> (ø)
R/helpers-testthat.R 100% <0%> (ø)
R/available-module-functions.R 100% <0%> (ø)
R/mcmc-scatterplots.R 100% <0%> (ø)
R/bayesplot-colors.R 100% <0%> (ø)

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 c8eb0d4...6bb1c2d. Read the comment docs.

@jgabry
Copy link
Member

jgabry commented Feb 28, 2017 via email

@avehtari
Copy link
Contributor

If you decide to have an option for showing the points, it would be useful to add some jitter in x direction, so that it's possible to see if there are several overlapping y values. This will work also for small number of observations.

@jgabry
Copy link
Member

jgabry commented Feb 28, 2017 via email

@silberzwiebel
Copy link
Contributor Author

Cool. I'll implement an option that let's the user specify if he/she wants to see y as violin, jittered points, or both, in the next week or so.
This would also add some more substance to this pull request that for now only changed 2 lines ;)

Regarding the documentation: Where would I need to put the description of the additional options so that it lands in the documentation of violin_grouped? Until now, I didn't code inside an R package.

@jgabry
Copy link
Member

jgabry commented Mar 1, 2017

Great, thanks!

The documentation for a new argument to ppc_violin_grouped can go right after these lines in the ppc-distributions.R file (right above the definition of the ppc_violin_grouped function). You can add it right below the documentation for the probs argument that's already there. The format is like this:

#' @param argname One or several sentences describing what the argument is used for. 

where argname is whatever the new argument name is.

Once you add that you can run

devtools::document()
devtools::load_all()
?ppc_violin_grouped

and you should see the updated documentation.

@jgabry jgabry added the feature label Mar 2, 2017
@silberzwiebel
Copy link
Contributor Author

Just added the jittered points and an option to toggle between plotting y as violin, jittered points or both. I've also added some more arguments to manipulate appearance of y (defaults based on best-looking for some of mine real-world models) and two examples. Tested everything via brms::pp_check(). Not sure, if I followed all potential code guideline, so feel free to edit where necessary.

@jgabry
Copy link
Member

jgabry commented Mar 13, 2017

I have some small changes I want to make but this looks good! I'm going to merge it now and then I'll make the changes, which is probably easier than me making commits to your branch

@jgabry jgabry merged commit 07af46b into stan-dev:master Mar 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants