Monitor population parameters

[sschildhauer]

• Analyze CI failures (docs-check and snapshot test failures)
• Fix attribute ordering in Run_Mod.R (class moved to position [8] instead of [2])
• Regenerate documentation with devtools::document()
• Verify tests pass locally

[codecov]

❌ 5 Tests Failed:

Tests completed	Failed	Passed	Skipped
29	5	24	0

View the top 3 failed test(s) by shortest run time

run_mod::results_are_consistent_with_unstratified_SEES_data_with_jags.post_included

Stack Traces | 0.022s run time

Error in `tibble(Iteration = NULL, Chain = NULL, value = NULL, Subject = NULL, Parameter = NULL, Iso_type = NULL, Stratification = NULL)`: could not find function "tibble"
Backtrace:
    x
 1. +-base::suppressWarnings(...) at test-run_mod.R:135:5
 2. | \-base::withCallingHandlers(...)
 3. \-serodynamics::run_mod(...)

run_mod::results_are_consistent_with_unstratified_SEES_data_with_modified_priors

Stack Traces | 0.023s run time

Error in `tibble(Iteration = NULL, Chain = NULL, value = NULL, Subject = NULL, Parameter = NULL, Iso_type = NULL, Stratification = NULL)`: could not find function "tibble"
Backtrace:
    x
 1. +-base::suppressWarnings(...) at test-run_mod.R:169:5
 2. | \-base::withCallingHandlers(...)
 3. \-serodynamics::run_mod(...)

run_mod::results_are_consistent_with_unstratified_SEES_data

Stack Traces | 0.035s run time

Error in `tibble(Iteration = NULL, Chain = NULL, value = NULL, Subject = NULL, Parameter = NULL, Iso_type = NULL, Stratification = NULL)`: could not find function "tibble"
Backtrace:
    x
 1. +-base::suppressWarnings(...) at test-run_mod.R:96:5
 2. | \-base::withCallingHandlers(...)
 3. \-serodynamics::run_mod(...)

run_mod::results_are_consistent_with_SEES_data

Stack Traces | 0.053s run time

Error in `tibble(Iteration = NULL, Chain = NULL, value = NULL, Subject = NULL, Parameter = NULL, Iso_type = NULL, Stratification = NULL)`: could not find function "tibble"
Backtrace:
    x
 1. +-base::suppressWarnings(...) at test-run_mod.R:57:5
 2. | \-base::withCallingHandlers(...)
 3. \-serodynamics::run_mod(...)

run_mod::results_are_consistent_with_simulated_data

Stack Traces | 0.286s run time

Error in `tibble(Iteration = NULL, Chain = NULL, value = NULL, Subject = NULL, Parameter = NULL, Iso_type = NULL, Stratification = NULL)`: could not find function "tibble"
Backtrace:
    x
 1. +-base::suppressWarnings(...) at test-run_mod.R:16:5
 2. | \-base::withCallingHandlers(...)
 3. \-serodynamics::run_mod(...)

To view more test analytics, go to the Test Analytics Dashboard
_{📋 Got 3 mins? Take this short survey to help us improve Test Analytics.}

[github-actions]

📖 https://ucd-serg.github.io/serodynamics/preview/pr141
Preview documentation for this PR (at commit 4163a8c)

[sschildhauer]

I am unsure why these final tests aren't passing. @Kwan-Jenny Any thoughts?

[Kwan-Jenny]

I am unsure why these final tests aren't passing. @Kwan-Jenny Any thoughts?

I am not sure what’s causing these final test failures. It may be helpful to ask @d-morrison for his thoughts.

[sschildhauer]

Hi @d-morrison , I have not seen this review fail message. I am a but confused how this came up as I made very few changes from the last review. It looks like it might have something to do with package dependencies, but unsure. Once this is done I can send up the posterior predictive check for review.

[Copilot]

Pull request overview

Copilot reviewed 23 out of 24 changed files in this pull request and generated 1 comment.

[Kwan-Jenny]

@copilot Please review the following comments and revise this PR accordingly.

1. Documentation out of sync — docs-check failure

The docs-check job fails at "Check for changed files" because man/expect_snapshot_data.Rd is stale.

Action needed:
Run devtools::document() locally and commit the updated .Rd file(s) in man/.

devtools::document()

Then:

git add man/
git commit -m "docs: regenerate Rd files after roxygen updates"
git push

---

2. Snapshot test failures (6 tests) — attribute ordering changed

All 6 snapshot failures (test-as_case_data.R:21, test-run_mod.R:29,69,108,148,186) share the same root cause: the order of object attributes has changed. The attribute class shifted from position [2] to the end [8], while every other attribute name remains identical.

Example diff (repeated across all 6):

     names(old)        | names(new)
 [1] names             | names             [1]
 [2] class             - nChains           [2]   # class was here
 [3] nChains           - nParameters       [3]
 ...
 [8] description       - class             [8]   # class moved here
 [9] population_params | population_params [9]
[10] priors            | priors            [10]

This is almost certainly caused by a change in how/when class() is assigned inside the constructor or helper that builds the result object. The actual content of attributes appears unchanged — only the insertion order differs.

Action needed — pick one:

Option A (recommended if the new ordering is intentional):
Accept the updated snapshots:

testthat::snapshot_accept()

Then commit the updated snapshot files under tests/testthat/_snaps/.

Option B (if the old ordering should be preserved):
Ensure class is assigned before the other attributes (e.g., nChains, nParameters, etc.) in the object constructor. Look for code like:

attr(result, "nChains") <- nChains
# ... other attrs ...
class(result) <- c("serodynamics_result", "data.frame")

and move the class() assignment to right after names:

class(result) <- c("serodynamics_result", "data.frame")
attr(result, "nChains") <- nChains
# ... rest ...

---

3. Windows & Ubuntu — population_params snapshot values diverge

On Windows (windows-latest) and Ubuntu (ubuntu-latest (devel), release, oldrel-1), the R CMD check logs show massive diffs in population_params numeric values (old vs new), with thousands of lines of changes. The log is truncated at ~125k lines.

This suggests the new code either:

• changed how population_params are computed/stored, or
• introduced platform-dependent numerical differences (floating-point ordering, RNG seed handling, etc.)

Action needed:

1. Confirm whether population_params content is expected to change with this PR. If yes → update snapshots on all platforms.
2. If the values should NOT have changed, check for:
   • Changes to the JAGS model specification
   • Changes to default RNG seeds or initial values
   • Changes to data subsetting/filtering that alter the input to the model
3. Consider whether snapshot tests on population_params (thousands of numeric values) are the right testing strategy. A tolerance-based comparison (expect_equal(..., tolerance = 1e-4)) may be more robust across platforms.

---

Summary of required actions

Priority	Issue	Fix
P0	man/expect_snapshot_data.Rd not regenerated	devtools::document() + commit
P0	6 snapshot failures (attribute order)	testthat::snapshot_accept() + commit, OR reorder class assignment
P1	population_params numeric diffs on Win/Linux	Investigate if intentional; update snapshots or add tolerance

---

[Copilot]

Pull request overview

Copilot reviewed 24 out of 25 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 25 out of 26 changed files in this pull request and generated 2 comments.

[Kwan-Jenny]

My Review

Hey @sschildhauer! Big step forward. I looked through the remaining CI failures on Windows and Ubuntu, and the good news is they're all coming from the same root cause: snapshot tests.

Right now the snapshot tests save exact text output (including numeric values from JAGS) and compare character-by-character. That works fine on macOS because you accepted snapshots there. But Windows and Ubuntu produce slightly different floating-point numbers — like 0.12345 vs 0.12346 — so the comparison fails even though the results are basically correct.

The test-coverage failure is just a side effect. Once the tests pass, coverage will work too.

My suggestion: Instead of saving separate snapshots for each OS, let's make the tests platform-independent. We can check attribute names with expect_setequal() (order doesn't matter) and compare numeric values with expect_equal(..., tolerance = ...) instead of exact text matching. That way we won't run into this problem again in the future.

---

Copilot Prompt (feel free to copy-paste this directly)

If you want, you can copy the block below and give it straight to Copilot to handle the refactoring:

The testthat snapshot tests in tests/testthat/test-run_mod.R and tests/testthat/test-as_case_data.R are failing on Windows and Ubuntu because the saved snapshots were generated on macOS. The numeric values from JAGS differ slightly across platforms due to floating-point behavior, causing character-level snapshot mismatches.

Please refactor these tests to be platform-independent:

1. Attribute checks: Replace expect_snapshot(rlist::list.remove(attributes(results), ...)) with assertions that check the attribute names exist as a set, regardless of order. For example, use expect_setequal(names(attributes(result)), expected_names) or expect_true(all(c("nChains", "nParameters", ...) %in% names(attributes(result)))).

2. Numeric value checks: Replace expect_snapshot() calls that compare population_params or other numeric outputs with expect_equal(..., tolerance = 1e-4) so that small floating-point differences across platforms are tolerated.

3. Keep non-numeric snapshots as-is if they are genuinely platform-independent (e.g., error messages, character strings).

4. Make sure the refactored tests still verify the same things: correct attribute names are present, numeric values are within a reasonable range, and the overall structure of the output is correct.

The affected test locations are approximately:

• test-as_case_data.R line 21
• test-run_mod.R lines 29, 69, 108, 148, 186

[Copilot]

Pull request overview

Copilot reviewed 29 out of 35 changed files in this pull request and generated 5 comments.

[Copilot]

jags_out is initialized with all columns set to NULL, which produces a tibble with zero columns. The first loop iteration then calls rbind(jags_out, jags_final), which will error due to mismatched column counts (and can also coerce types unexpectedly). Initialize jags_out with typed zero-length columns (e.g., integer(), character(), double()) or accumulate per-stratum results in a list and combine with dplyr::bind_rows() at the end (or use dplyr::bind_rows() each iteration starting from NULL).

Suggested change

	"Iteration" = NULL,
	"Chain" = NULL,
	"value" = NULL,
	"Subject" = NULL,
	"Parameter" = NULL,
	"Iso_type" = NULL,
	"Stratification" = NULL
	"Iteration" = integer(),
	"Chain" = integer(),
	"value" = numeric(),
	"Subject" = character(),
	"Parameter" = character(),
	"Iso_type" = character(),
	"Stratification" = character()

[Copilot]

The population-parameter test compares summary stats to a single reference ref_popparam_summary_stats.rds. This is likely to be platform-dependent (the repo already uses darwin_variant() because JAGS MCMC output differs on macOS), so this expectation may be flaky across CI OSes. Consider switching to expect_snapshot_data(..., variant = darwin_variant()) for the summary output, or storing per-variant reference files and selecting the appropriate one at runtime.

[Copilot]

The comment says this ensures class appears at position [2] “after names and row.names”, but if row.names is present then class is actually the 3rd attribute. Consider rewording the comment to avoid an off-by-one/ambiguous claim (e.g., “immediately after names and row.names”).

Suggested change

	# Explicitly build the attribute list in the correct order to ensure
	# that `class` appears at position [2] (after `names` and `row.names`).
	# Explicitly build the attribute list in the correct order to ensure that
	# `class` appears immediately after `names` and `row.names`.

[Copilot]

The comment says class should be at position [2] “after names and row.names”, but with row.names present class will be the 3rd attribute. Rewording this comment would make it clearer what ordering constraint is intended (e.g., “immediately after names and row.names”).

Suggested change

	# Explicitly order attributes to ensure class is at position [2]
	# (after names and row.names), which is the expected position for
	# tibble subclasses. tibble::as_tibble() on a subclass can reorder
	# Explicitly order attributes so that class comes immediately after
	# names and row.names, which is the expected position for tibble
	# subclasses. tibble::as_tibble() on a subclass can reorder

[Copilot]

with_pop_params defaults to TRUE, which adds mu.par, prec.par, and prec.logy to monitor. Monitoring these (especially prec.par) can substantially increase the size of the MCMC object returned by JAGS and slow down runs / increase memory use for all callers. Consider making this opt-in (default FALSE) or, if the default must remain TRUE, clearly documenting the performance/memory impact in the function docs and NEWS so users can disable it when they don’t need population parameters.