Skip to content

add ppc_*_overlay_grouped functions #212

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

Merged
merged 8 commits into from
Dec 3, 2020
Merged

Conversation

tjmahr
Copy link
Collaborator

@tjmahr tjmahr commented Dec 2, 2019

library(bayesplot)
#> This is bayesplot version 1.7.1
#> - Online documentation and vignettes at mc-stan.org/bayesplot
#> - bayesplot theme set to bayesplot::theme_default()
#>    * Does _not_ affect other ggplot2 plots
#>    * See ?bayesplot_theme_set for details on theme setting
y <- example_y_data()
yrep <- example_yrep_draws()
group <- example_group_data()

ppc_dens_overlay_grouped(y, yrep[1:20,], group = group)

ppc_ecdf_overlay_grouped(y, yrep[1:20,], group = group)

Created on 2019-12-02 by the reprex package (v0.3.0)

@tjmahr
Copy link
Collaborator Author

tjmahr commented Dec 2, 2019

The plots now have axis lines. (Note the addition of the vertical y-axis lines compared to the original versions above.)

library(bayesplot)
#> This is bayesplot version 1.7.1
#> - Online documentation and vignettes at mc-stan.org/bayesplot
#> - bayesplot theme set to bayesplot::theme_default()
#>    * Does _not_ affect other ggplot2 plots
#>    * See ?bayesplot_theme_set for details on theme setting
y <- example_y_data()
yrep <- example_yrep_draws()
group <- example_group_data()

ppc_dens_overlay_grouped(y, yrep[1:20,], group = group)

ppc_ecdf_overlay_grouped(y, yrep[1:20,], group = group)

Created on 2019-12-02 by the reprex package (v0.3.0)

@tjmahr
Copy link
Collaborator Author

tjmahr commented Dec 2, 2019

Ready for review.

Travis won't build it because of BH issues and rhub fails when trying to install BiocManager but the checks on my Windows 10 machine all pass.

I do get warnings during tests about our use of expand_scale(), but that's because I have the dev version of ggplot2 where expand_scale() has been deprecated.

@tjmahr tjmahr changed the title [WIP] add ppc_*_overlay_grouped functions add ppc_*_overlay_grouped functions Dec 2, 2019
@tjmahr
Copy link
Collaborator Author

tjmahr commented Dec 9, 2019

Oops, this breaks code from the brms package.

pp_check(model)
 Error: Argument 'group' is required for ppc type 'dens_overlay'. 

@jgabry
Copy link
Member

jgabry commented May 15, 2020

Wow, sorry it took me forever to get to this @tjmahr! This looks great.

Oops, this breaks code from the brms package.

@paul-buerkner is there something in brms that can be changed for this to not cause an error?

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@jgabry
Copy link
Member

jgabry commented Jun 16, 2020

@tjmahr Coming back to this, I looked into the brms issue and I think the reason this causes an error in brms is that as part of this PR ppc_dens_overlay() gains a group argument too (instead of just the ppc_dens_overlay_grouped() function having the group argument. brms thinks any functions with the group argument require the group argument (which has always been the case).

@jgabry
Copy link
Member

jgabry commented Jun 16, 2020

I think if we wait on this until we deal with #151 then we can use the new mechanism for the grouped plots calling the ungrouped version. That should get around the brms error.

@jgabry jgabry added the feature label Jun 21, 2020
@jgabry
Copy link
Member

jgabry commented Dec 3, 2020

pp_check(model)
Error: Argument 'group' is required for ppc type 'dens_overlay'.

#253 which was just opened reminded me of this. @tjmahr is there a reason ppc_dens_overlay needs to gain a group argument? Can that argument exclusively be in ppc_dens_overlay_grouped? That would get around the brms error that was preventing us from merging this. Otherwise we might need to wait for the big PR #222 before merging this.

@jgabry jgabry mentioned this pull request Dec 3, 2020
@jgabry jgabry linked an issue Dec 3, 2020 that may be closed by this pull request
@tjmahr
Copy link
Collaborator Author

tjmahr commented Dec 3, 2020

library(bayesplot)
#> This is bayesplot version 1.7.2.9000
#> - Online documentation and vignettes at mc-stan.org/bayesplot
#> - bayesplot theme set to bayesplot::theme_default()
#>    * Does _not_ affect other ggplot2 plots
#>    * See ?bayesplot_theme_set for details on theme setting

y <- example_y_data()
yrep <- example_yrep_draws()
group <- example_group_data()

ppc_dens_overlay_grouped(y, yrep[1:20,], group = group)

y <- example_y_data()
yrep <- example_yrep_draws()
group <- example_group_data()

ppc_ecdf_overlay_grouped(y, yrep[1:20,], group = group)

Created on 2020-12-03 by the reprex package (v0.3.0)

@tjmahr
Copy link
Collaborator Author

tjmahr commented Dec 3, 2020

pp_check(model)
Error: Argument 'group' is required for ppc type 'dens_overlay'.

#253 which was just opened reminded me of this. @tjmahr is there a reason ppc_dens_overlay needs to gain a group argument? Can that argument exclusively be in ppc_dens_overlay_grouped? That would get around the brms error that was preventing us from merging this. Otherwise we might need to wait for the big PR #222 before merging this.

I removed the group argument from ppc_dens_overlay() and used a ggplot2 idiom to keep from writing the same plotting code twice.

# data in the y and yrep layers should be safe because they are
# specified using a function on the main plot data.
data <- ppc_data(y, yrep, group = group)
p_overlay <- p_overlay + list(data)
Copy link
Member

Choose a reason for hiding this comment

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

I didn't even know about this trick!

Copy link
Member

Choose a reason for hiding this comment

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

Or I maybe did but had since forgotten. Either way, this seems seems good.

Copy link
Member

@jgabry jgabry left a comment

Choose a reason for hiding this comment

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

Thanks for updating this @tjmahr. I think it's ready to go but will let all the checks finish running.

@jgabry jgabry merged commit cb1b403 into master Dec 3, 2020
@jgabry jgabry deleted the ppc-dens-overlay-group branch December 3, 2020 23:22
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.

ppc_dens_overlay_grouped
2 participants