Forward bounds argument #405
Conversation
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 5786 -339
==========================================
- Hits 6043 5706 -337
+ 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.
R/ppc-loo.R
Outdated
| trim = FALSE, | ||
| adjust = 1, | ||
| kernel = "gaussian", | ||
| bounds = NULL, |
There was a problem hiding this comment.
I don't think we need bounds for ppc_loo_pit_overlay() since it's always bounded between 0 and 1 and I don't think it's particularly useful to restrict it even further.
|
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>
Closes #317.