Fix abtest bayesian - #323
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR aims to fix the ABTest Bayesian analysis by adapting the input passed to abtest::ab_test() and updating test snapshots accordingly.
Changes:
- Rename/massage input data so
abtest::ab_test()receives expectedy1/n1/y2/n2fields. - Update prior/posterior pie chart labeling to avoid
coord_polar()rendering differences. - Regenerate snapshot SVGs and update
renv.lockto a newerjaspBasecommit.
Reviewed changes
Copilot reviewed 2 out of 57 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
R/abtestbayesian.R |
Adjusts dataset column handling and switches ab_test() input to a named list; tweaks pie chart labeling. |
tests/testthat/test-abtestbayesian.R |
Changes macOS skip behavior for a snapshot-based plot test. |
tests/testthat/_snaps/**.svg |
Updates snapshot outputs to match new rendering/layout. |
renv.lock |
Bumps jaspBase remote SHA to align dependency state with new behavior. |
Comments suppressed due to low confidence (1)
tests/testthat/test-abtestbayesian.R:1
- Leaving
skip_on_os(\"mac\")commented out makes the test behavior ambiguous and can lead to unexpected CI failures on macOS without documenting why. Either remove the line entirely (if macOS is now supported) or re-enable the skip with a clear comment explaining the specific macOS failure mode and (ideally) the condition under which it can be removed.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (ready) { | ||
| # ab_test expects data with columns named n1, n2, y1, y2 | ||
| colnames(dataset) <- stringr::str_replace_all( | ||
| colnames(dataset), | ||
| setNames(c("n1", "n2", "y1", "y2"), c(options$n1, options$n2, options$y1, options$y2)) | ||
| ) | ||
| } |
There was a problem hiding this comment.
Mutating colnames(dataset) to n1/n2/y1/y2 can break other code paths that still expect the original column names via options$n1/options$y1/... (e.g., descriptives tables or any downstream access by the selected variable names). Also, stringr::str_replace_all() uses regex/substitution and can rename unintended columns if options$* values are substrings of other column names. Prefer constructing data_list directly from the selected columns (using dataset[[options$y1]], etc.) without renaming dataset, or (if renaming is unavoidable) rename via exact-name matching on a copy (e.g., dataset_ab <- dataset) rather than regex replacement.
| p_prior <- p_prior[names(hyp_index2)] | ||
| p_post <- p_post [names(hyp_index2)] | ||
|
|
||
| # use NonPolar variant to avoid coord_polar rendering issues in newer ggplot2 |
There was a problem hiding this comment.
The comment says a “NonPolar variant” is being used, but the code still calls jaspGraphs::plotPieChart() and simply removes the explicit coord_polar() call. Please update the comment to accurately describe what changed, or switch to the actual non-polar plotting helper (if one exists in jaspGraphs) so the comment matches the implementation.
| # use NonPolar variant to avoid coord_polar rendering issues in newer ggplot2 | |
| # continue to use jaspGraphs::plotPieChart(); this code path avoids adding an | |
| # extra explicit coord_polar() layer here |
8fdf994 to
ce532fb
Compare
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
ce532fb to
6ee9ff4
Compare
Also add plot fallback files