Add tests for add_userdata() and fix single-chain handling by Copilot · Pull Request #33 · USCbiostats/fmcmc

Copilot · 2025-12-09T07:13:08Z

Plan for adding tests for add_userdata()

💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

- Fixed add_userdata() to handle single chain mcmc objects - Added examples to documentation with roxygen2 - Created 5 comprehensive test cases covering: * Basic single chain functionality * Multiple chain support * Data integrity verification * Error handling for mismatched chains * mcpar attribute preservation - Updated NAMESPACE and documentation files Co-authored-by: gvegayon <893619+gvegayon@users.noreply.github.com>

… mcmc objects Co-authored-by: gvegayon <893619+gvegayon@users.noreply.github.com>

Copilot · 2025-12-09T07:33:12Z

Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

CRAN.R-project.org
- Triggering command: /opt/R/4.5.2/lib/R/bin/exec/R /opt/R/4.5.2/lib/R/bin/exec/R --no-restore --no-echo --args nextArgfmcmc_0.6-0.tar.gznextArg--as-crannextArg--no-manualnextArg--no-build-vignettesnextArg--no-manual (dns block)
bioconductor.org
- Triggering command: /opt/R/4.5.2/lib/R/bin/exec/R /opt/R/4.5.2/lib/R/bin/exec/R --no-restore --no-echo --args nextArgfmcmc_0.6-0.tar.gznextArg--as-crannextArg--no-manualnextArg--no-build-vignettesnextArg--no-manual (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Configure Actions setup steps to set up my environment, which run before the firewall is enabled
Add the appropriate URLs or hosts to the custom allowlist in this repository's Copilot coding agent settings (admins only)

Copilot

Pull request overview

This PR fixes add_userdata() to properly handle single-chain mcmc objects (previously it only worked with mcmc.list), and adds comprehensive test coverage with 35 new tests.

Converts single-chain mcmc objects to mcmc.list internally for processing, then returns the appropriate type
Handles the case where get_userdata() returns a data.frame for single chains instead of a list
Adds 5 test suites covering basic functionality, data integrity, error handling, and mcpar preservation

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File	Description
R/mcmc_info.R	Implements single-chain detection, conversion logic, and proper return type handling in `add_userdata()`
inst/tinytest/test-userdata.R	Adds comprehensive test coverage including single/multi-chain tests, data integrity checks, and edge case validation
man/mcmc-loop.Rd	Updates documentation to reflect new single-chain support and adds usage examples
NAMESPACE	Adds necessary coda imports (`as.mcmc.list`, `mcpar`) for the new functionality

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-09T17:11:28Z

+combined mcmc.list (or mcmc if input was a single chain mcmc object)
 }
 \description{
 You can use these functions to read variables, store, and retrieve data
 during the MCMC process.
+
+Combines list of dataframes produced by fmcmc::get_userdata()
+with mcmc.list() produced by fmcmc::MCMC ensuring that the
+chains and iterations match


The documentation for add_userdata() is placed in the middle of the \value{} section for all functions in this help page. This creates unclear documentation where the return value description "combined mcmc.list (or mcmc if input was a single chain mcmc object)" appears disconnected from the function it describes.

The description section also now contains two separate descriptions - one for the general mcmc-loop functions and one specifically for add_userdata(). This makes the documentation confusing to read.

Consider restructuring the documentation so that add_userdata() has its own dedicated documentation page with @rdname add_userdata instead of grouping it with the other mcmc-loop functions, OR ensure the descriptions and return values are properly organized in the shared help page.

Copilot · 2025-12-09T17:11:29Z

+\item{x}{An mcmc or mcmc.list object to add userdata to. Note, that userdata
+is taken from the environment (whatever fmcmc::get_userdata()
+returns)}



The parameter documentation for x has been replaced but now only describes the add_userdata() function's parameter. This documentation is shared across multiple functions (ith_step, set_userdata, get_userdata, and add_userdata), but ith_step(x) also takes an x parameter with a completely different meaning (the name of an element to retrieve).

The previous documentation described ith_step's parameter: "Name of the element to retrieve. If missing, it will return the entire environment in which the main MCMC loop is running."

This change makes the documentation incorrect for ith_step(x). Each function should have its parameter documented separately, or the documentation should cover all uses of the parameter.

Suggested change

\item{x}{An mcmc or mcmc.list object to add userdata to. Note, that userdata

is taken from the environment (whatever fmcmc::get_userdata()

returns)}

\item{x}{

For \code{ith_step(x)}: Name of the element to retrieve from the MCMC environment. If missing, returns the entire environment in which the main MCMC loop is running.

For \code{add_userdata(x)}: An mcmc or mcmc.list object to add userdata to. Note that userdata is taken from the environment (whatever \code{fmcmc::get_userdata()} returns).

}

Copilot · 2025-12-09T17:11:29Z

+# Test 4: Edge case - check error when number of chains mismatch --------------
+# Create a scenario where get_userdata() doesn't match the mcmc.list
+# This tests the validation in add_userdata()
+
+set.seed(111)
+n_edge1 <- 20
+
+fun_edge1 <- function(p) {
+  i <- ith_step("i")
+  set_userdata(idx = i)
+  dnorm(p, log = TRUE)
+}
+
+ans_edge1 <- MCMC(
+  fun     = fun_edge1,
+  initial = 0,
+  nsteps  = n_edge1,
+  nchains = 2,
+  kernel  = kernel_normal(scale = 1),
+  seed    = 111
+)
+
+# Now run a different MCMC with different number of chains
+# This will overwrite the userdata with different chain count
+ans_edge1_wrong <- MCMC(
+  fun     = fun_edge1,
+  initial = 0,
+  nsteps  = n_edge1,
+  nchains = 3,  # Different number of chains
+  kernel  = kernel_normal(scale = 1),
+  seed    = 222
+)
+
+# Test: should error because number of chains don't match
+# ans_edge1 has 2 chains but get_userdata() now returns 3 chains
+expect_error(
+  add_userdata(ans_edge1),
+  "Number of chains does not match"
+)


Test 4 verifies error handling when the number of chains in the mcmc object doesn't match the userdata. However, the test relies on global state (MCMC_OUTPUT) being overwritten by running a second MCMC. This creates fragile test behavior:

The test assumes get_userdata() will return the userdata from ans_edge1_wrong (3 chains) when called within add_userdata(ans_edge1)

This works only because MCMC_OUTPUT is global and the second MCMC overwrites the first

If tests are run in a different order or isolation, this test could fail unexpectedly

A more robust test would explicitly set up a mismatch scenario without relying on side effects, or at least document this global state dependency clearly in comments.

You could add a note on the documentation mentioning this. I think the key is to ensure the user is aware that the add_userdata() relies on the latest run of MCMC().

Copilot · 2025-12-09T17:11:30Z

+combined_mcpar <- add_userdata(ans_mcpar)
+
+# Test: mcpar attributes should match
+orig_mcpar <- coda::mcpar(ans_mcpar)
+combined_mcpar_attr <- coda::mcpar(combined_mcpar)


Variable naming inconsistency: the test uses combined_mcpar both for the result of add_userdata() (line 248) and for the mcpar attribute extracted from that result (line 252).

Line 248: combined_mcpar <- add_userdata(ans_mcpar) creates an mcmc object
Line 252: combined_mcpar_attr <- coda::mcpar(combined_mcpar) extracts the attribute

The name combined_mcpar is misleading since it contains the combined data, not just the mcpar attribute. Consider renaming the variable on line 248 to something like combined_data or combined_result for clarity.

Suggested change

combined_mcpar <- add_userdata(ans_mcpar)

# Test: mcpar attributes should match

orig_mcpar <- coda::mcpar(ans_mcpar)

combined_mcpar_attr <- coda::mcpar(combined_mcpar)

combined_data <- add_userdata(ans_mcpar)

# Test: mcpar attributes should match

orig_mcpar <- coda::mcpar(ans_mcpar)

combined_mcpar_attr <- coda::mcpar(combined_data)

Sounds like a good suggestion

gvegayon

@copilot, address the comments made here.

gvegayon · 2025-12-09T18:37:59Z

+# Test 4: Edge case - check error when number of chains mismatch --------------
+# Create a scenario where get_userdata() doesn't match the mcmc.list
+# This tests the validation in add_userdata()
+
+set.seed(111)
+n_edge1 <- 20
+
+fun_edge1 <- function(p) {
+  i <- ith_step("i")
+  set_userdata(idx = i)
+  dnorm(p, log = TRUE)
+}
+
+ans_edge1 <- MCMC(
+  fun     = fun_edge1,
+  initial = 0,
+  nsteps  = n_edge1,
+  nchains = 2,
+  kernel  = kernel_normal(scale = 1),
+  seed    = 111
+)
+
+# Now run a different MCMC with different number of chains
+# This will overwrite the userdata with different chain count
+ans_edge1_wrong <- MCMC(
+  fun     = fun_edge1,
+  initial = 0,
+  nsteps  = n_edge1,
+  nchains = 3,  # Different number of chains
+  kernel  = kernel_normal(scale = 1),
+  seed    = 222
+)
+
+# Test: should error because number of chains don't match
+# ans_edge1 has 2 chains but get_userdata() now returns 3 chains
+expect_error(
+  add_userdata(ans_edge1),
+  "Number of chains does not match"
+)


You could add a note on the documentation mentioning this. I think the key is to ensure the user is aware that the add_userdata() relies on the latest run of MCMC().

gvegayon · 2025-12-09T18:38:39Z

+combined_mcpar <- add_userdata(ans_mcpar)
+
+# Test: mcpar attributes should match
+orig_mcpar <- coda::mcpar(ans_mcpar)
+combined_mcpar_attr <- coda::mcpar(combined_mcpar)


Sounds like a good suggestion

@PARAM

- Fix @PARAM x documentation to cover both ith_step() and add_userdata() - Add important note that add_userdata() relies on most recent MCMC() call - Rename combined_mcpar to combined_data for clarity in test - Add comment explaining global state dependency in Test 4 Co-authored-by: gvegayon <893619+gvegayon@users.noreply.github.com>

Copilot · 2025-12-09T18:45:43Z

@copilot, address the comments made here.

Addressed all review comments in commit fadb36f:

Fixed @param x documentation to describe both ith_step(x) and add_userdata(x) parameters
Added important note in documentation that add_userdata() relies on the most recent MCMC() call
Renamed combined_mcpar variable to combined_data for clarity
Added comment explaining global state dependency in Test 4

gvegayon

LGTM!

@PARAM

* introduce add_userdata() (#23) * introduce add_userdata() * Update R/mcmc_info.R Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Update R/mcmc_info.R Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Add tests for add_userdata() and fix single-chain handling (#33) * Initial plan * Add comprehensive tests for add_userdata() function - Fixed add_userdata() to handle single chain mcmc objects - Added examples to documentation with roxygen2 - Created 5 comprehensive test cases covering: * Basic single chain functionality * Multiple chain support * Data integrity verification * Error handling for mismatched chains * mcpar attribute preservation - Updated NAMESPACE and documentation files Co-authored-by: gvegayon <893619+gvegayon@users.noreply.github.com> * Fix test-userdata.R: remove incorrect [[1]] indexing for single chain mcmc objects Co-authored-by: gvegayon <893619+gvegayon@users.noreply.github.com> * Address PR review comments - Fix @PARAM x documentation to cover both ith_step() and add_userdata() - Add important note that add_userdata() relies on most recent MCMC() call - Rename combined_mcpar to combined_data for clarity in test - Add comment explaining global state dependency in Test 4 Co-authored-by: gvegayon <893619+gvegayon@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: gvegayon <893619+gvegayon@users.noreply.github.com> * [WIP] Add test for add_userdata() function (#34) * Initial plan * Fix test-userdata.R: compare against proposed values, not accepted values The test was incorrectly comparing userdata stored during MCMC proposal evaluation against the final accepted parameter values. Since set_userdata is called with the proposed value p (not the accepted value), the test should compare against get_draws() (proposed values) rather than the MCMC output (accepted values). Co-authored-by: gvegayon <893619+gvegayon@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: gvegayon <893619+gvegayon@users.noreply.github.com> --------- Co-authored-by: Deemah <dperepolkin@gmail.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com> Co-authored-by: gvegayon <893619+gvegayon@users.noreply.github.com>

Initial plan

890d956

Copilot AI assigned Copilot and gvegayon Dec 9, 2025

Copilot AI mentioned this pull request Dec 9, 2025

introduce add_userdata() (#23) #32

Merged

Copilot started work on behalf of gvegayon December 9, 2025 07:13 View session

Copilot AI and others added 2 commits December 9, 2025 07:30

Fix test-userdata.R: remove incorrect [[1]] indexing for single chain…

959833a

… mcmc objects Co-authored-by: gvegayon <893619+gvegayon@users.noreply.github.com>

Copilot AI changed the title ~~[WIP] Add test for add_userdata function~~ Add tests for add_userdata() and fix single-chain handling Dec 9, 2025

Copilot AI requested a review from gvegayon December 9, 2025 07:33

Copilot finished work on behalf of gvegayon December 9, 2025 07:33

gvegayon marked this pull request as ready for review December 9, 2025 07:51

gvegayon requested a review from Copilot December 9, 2025 17:06

Copilot started reviewing on behalf of gvegayon December 9, 2025 17:07 View session

Copilot AI reviewed Dec 9, 2025

View reviewed changes

gvegayon requested changes Dec 9, 2025

View reviewed changes

Copilot started work on behalf of gvegayon December 9, 2025 18:39 View session

Copilot AI requested a review from gvegayon December 9, 2025 18:48

Copilot finished work on behalf of gvegayon December 9, 2025 18:48

gvegayon approved these changes Dec 9, 2025

View reviewed changes

gvegayon merged commit 14d530e into dmi3kmo/new_feat Dec 9, 2025
1 check passed

gvegayon deleted the copilot/sub-pr-32 branch December 9, 2025 19:13

Uh oh!

Conversation

Copilot AI commented Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Plan for adding tests for add_userdata()

Uh oh!

Copilot AI commented Dec 9, 2025

I tried to connect to the following addresses, but was blocked by firewall rules:

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

gvegayon Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

gvegayon Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

gvegayon left a comment

Choose a reason for hiding this comment

Uh oh!

gvegayon Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

gvegayon Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI commented Dec 9, 2025

Uh oh!

gvegayon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Copilot AI commented Dec 9, 2025 •

edited

Loading