Allow passing bounds argument to ggplot2 for density plots#405
Allow passing bounds argument to ggplot2 for density plots#405
bounds argument to ggplot2 for density plots#405Conversation
Update branch
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #405 +/- ##
==========================================
- Coverage 98.66% 98.61% -0.05%
==========================================
Files 35 35
Lines 6125 5784 -341
==========================================
- Hits 6043 5704 -339
+ Misses 82 80 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR adds a bounds argument to density plotting functions across the bayesplot package, allowing users to truncate the density support when visualizing posterior and predictive distributions. The change closes issue #317 and provides consistent bounds support across PPC, PPD, and MCMC plotting functions.
Key Changes:
- Added a new validation helper function
validate_density_bounds()to ensure bounds are properly formatted - Extended function signatures to accept the
boundsparameter across all density-based plotting functions - Updated documentation to describe the bounds parameter and its usage
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| R/helpers-shared.R | Adds validate_density_bounds() helper function to validate bounds input |
| R/ppc-distributions.R | Updates ppc_dens, ppc_dens_overlay, and ppc_dens_overlay_grouped to accept and forward bounds argument |
| R/ppd-distributions.R | Updates ppd_dens and ppd_dens_overlay to accept and forward bounds argument |
| R/ppc-loo.R | Updates ppc_loo_pit_overlay to accept and forward bounds argument |
| R/mcmc-distributions.R | Updates MCMC density functions (mcmc_dens, mcmc_dens_overlay, mcmc_dens_chains, mcmc_violin) to accept and forward bounds argument |
| R/mcmc-intervals.R | Updates mcmc_areas, mcmc_areas_ridges and related data functions to accept bounds; modifies compute_interval_density to apply bounds constraints |
| man-roxygen/args-density-controls.R | Updates parameter documentation template to include bounds argument |
| man/PPC-distributions.Rd | Regenerated documentation reflecting bounds parameter additions |
| man/PPD-distributions.Rd | Regenerated documentation reflecting bounds parameter additions |
| man/PPC-loo.Rd | Regenerated documentation reflecting bounds parameter additions |
| man/MCMC-distributions.Rd | Regenerated documentation reflecting bounds parameter additions |
| man/MCMC-intervals.Rd | Regenerated documentation reflecting bounds parameter additions |
| tests/testthat/test-ppc-distributions.R | Adds tests verifying bounds argument works with density PPC/PPD plots |
| tests/testthat/test-mcmc-distributions.R | Adds tests verifying bounds argument works with MCMC density plots |
| tests/testthat/test-mcmc-intervals.R | Adds tests verifying bounds argument works with MCMC areas and ridges plots |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| validate_density_bounds <- function(bounds) { | ||
| if (is.null(bounds)) { | ||
| return(NULL) | ||
| } | ||
| if (!is.numeric(bounds) || length(bounds) != 2 || anyNA(bounds)) { | ||
| abort("`bounds` must be a numeric vector of length 2.") | ||
| } | ||
| if (bounds[1] >= bounds[2]) { | ||
| abort("`bounds` must satisfy bounds[1] < bounds[2].") | ||
| } | ||
| bounds | ||
| } |
There was a problem hiding this comment.
The new validate_density_bounds function lacks test coverage. While tests verify that the bounds argument can be passed to various plotting functions (e.g., in test-ppc-distributions.R, test-mcmc-distributions.R), there are no tests that validate the behavior of validate_density_bounds itself. Consider adding tests to test-helpers-shared.R to verify that the function correctly handles invalid inputs such as: non-numeric values, vectors with length != 2, vectors with NA values, and cases where bounds[1] >= bounds[2].
There was a problem hiding this comment.
You don't necessarily need to test the validate_density_bounds function itself, but it would be good to at least add a test where invalid bounds are passed to a plotting function.
There was a problem hiding this comment.
@copilot Implement Jonah's suggestion by writing a test in the same style as the existing tests where a plotting function receives invalid bounds. Test all branches of the validate_density_bounds function
There was a problem hiding this comment.
I'm not sure if that works, but I'll write/generate a test either way.
|
Looking good, thanks! I made one comment and one addition comment on copilot's comment. I also added a couple of visual tests and suppressed a bunch of warnings in the tests (it was generating so many warnings). |
|
@VisruthSK I've opened a new pull request, #406, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@jgabry Thanks for reviewing even when I forgot to write patch notes, I'll fix those two things. Also thanks for getting rid of the warnings I didn't see those. |
* Initial plan * Add tests for invalid bounds in plotting functions Co-authored-by: VisruthSK <67435125+VisruthSK@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: VisruthSK <67435125+VisruthSK@users.noreply.github.com>
jgabry
left a comment
jgabry
left a comment
There was a problem hiding this comment.
I think this is ready to go. Thanks @VisruthSK. Tests are running again, but once they pass whoever notices that they passed first can just go ahead and merge.
jgabry
changed the title
bounds argument to ggplot2 for density plots
Closes #317.