Debug calcfitmod

[sschildhauer]

Debugging calc_fit_mod in two ways:

• Currently calc_fit_mod takes a filtered raw data set in run_mod that calculates the fitted and residuals for only a subset of the data. This PR changes this piece to take the full raw input data set and calculate fitted and residuals for the whole data set.
• This PR also adds Stratification as a variable in the fitted_residuals output.
• Linking to issues Fix comments on calc_fit_mod #226 and Bug: calc_fit_mod uses only the last stratum's data (R/Run_Mod.R:268–271) #215

[claude]

Claude finished @sschildhauer's task in 2m 49s —— View job

---

Code Review: PR #240 — Debug calcfitmod

• Read CLAUDE.md (none found at root)
• Examine PR diff (origin/main...HEAD)
• Review R/calc_fit_mod.R
• Review R/Run_Mod.R
• Review documentation and snapshots
• Post final code review

---

Summary

The two-part fix is correct and well-implemented:

Before	After
calc_fit_mod() received dl_sub (filtered to current stratum each iteration); last stratum silently overwrote all prior strata	calc_fit_mod() receives full data once, together with strat, and merges against all strata's posterior estimates in a single pass
No Stratification column in fitted_residuals output	Stratification always present — stratum label when stratified, "None" when not

The implementation correctly leverages the existing Stratification column already present in jags_out (set via jags_final$Stratification <- i in the loop), making the merge key (Subject, Iso_type, Stratification) natural and unambiguous.

---

Correctness Analysis

R/calc_fit_mod.R — looks good

• Unstratified path (is.na(strat) == TRUE):

  • original_data is selected down to Subject, Iso_type, t, result.
  • modeled_dat summarizes and pivots, leaving one row per (Subject, Iso_type, Stratification="None").
  • merge(..., by = c("Subject", "Iso_type"), all.y = TRUE) carries Stratification = "None" from modeled_dat into the output. This is the right approach — Stratification doesn't come from original_data here, which is intentional and correct.
  • select(.data$Stratification, ...) at line 75 works because Stratification is always in modeled_dat.

• Stratified path (!is.na(strat)):

  • original_data gets the strat column selected and renamed to "Stratification" (guarded for name collision at line 44).
  • Merge key includes "Stratification", so each observation matches its stratum's posterior estimates.
  • all.y = TRUE correctly surfaces rows with NA strat values as NA fitted/residual (documented at lines 24–27).

• prep_strat_list: Confirmed that when strat = NA, it returns "None", so i = "None" in the loop → jags_final$Stratification = "None" → modeled_dat$Stratification = "None" after bind_rows. The whole chain is internally consistent.

R/Run_Mod.R:237–239 — looks good

Passing original_data = data (full dataset) and strat = strat is the minimal, correct change. The per-stratum filtering loop above it now only affects modeling; fitted/residual computation is decoupled and operates on the full data.

---

Findings

🟡 Minor doc inconsistency (non-blocking)

R/Run_Mod.R:54 and :87 both write `None` (unquoted), while R/calc_fit_mod.R:17 correctly writes `"None"` (with quotes). Since the actual R character value is "None", the quoted form is more precise and should be consistent across both files.

- - The default value for no stratification is `None`.
+ - The default value for no stratification is `"None"`.

- stratification will be `None`.
+ stratification will be `"None"`.

Fix this →

🟢 Correctness of NA-strat behavior

Rows with NA in the stratification column are warned about and dropped from strat_list in prep_strat_list. They are therefore not modeled, but since calc_fit_mod receives the full data, those rows appear in matched_dat (via all.y = TRUE) with NA fitted/residual. This is clearly documented at R/calc_fit_mod.R:24–27. Behavior is correct.

🟢 Name-collision guard (R/calc_fit_mod.R:44)

if (strat != "Stratification") {
  original_data <- original_data |>
    dplyr::rename("Stratification" = dplyr::all_of(strat))
}

This correctly handles the edge case where the user's stratification column is already named "Stratification". Clean.

🟢 Snapshots

Both darwin/ and root snapshot variants are present and consistent. The strat-fitted_residuals.csv correctly expanded from 214 to 904 data rows (covering all strata, not just the last one — exactly the bug being fixed). Column order Subject, Iso_type, Stratification, t, fitted, residual matches R/calc_fit_mod.R:75.

🟢 WORDLIST

allowlist restored (was an unrelated removal in a prior commit), calc added (required because spell_check_package() tokenizes calc_fit_mod in NEWS.md). Both entries are warranted.

🟢 NEWS.md

run_mod()'s fitted_residuals attribute now covers all observations across all strata (previously only the last stratum was retained) and always includes a Stratification column ("None" when unstratified). (#240)

Accurate and informative. Well done.

---

Verdict

Approve-with-nit. The core fix is correct, well-tested, and CI is green on all platforms. Only one non-blocking nit: harmonize the `None` / `"None"` quoting in R/Run_Mod.R to match R/calc_fit_mod.R. Nothing blocks merge.
• Branch: debug_calcfitmod

[github-actions]

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

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

Files with missing lines	Coverage Δ
R/Run_Mod.R	100.00% <100.00%> (ø)
R/calc_fit_mod.R	100.00% <100.00%> (ø)

[sschildhauer]

@claude, please help me fix this PR to pass all reviews. Please keep all edits within this thread.

[claude]

Claude finished @sschildhauer's task in 12m 11s —— View job

---

Fixed: regenerated the missing Linux snapshots

Root cause: The 'by' must specify a uniquely valid column merge error shown in the codecov comment was stale — it came from a commit before the calc_fit_mod merge logic was fixed. I pulled the live R-CMD-check log for the latest commit (d086af6, ubuntu R 4.6.0) and confirmed all 8 failures were simply "Adding new file snapshot":

• The PR deleted the non-darwin snapshot files under tests/testthat/_snaps/run_mod/ but never regenerated them.
• macOS CI passed because its darwin snapshots were updated; Linux & Windows look for the root (non-darwin) files, which no longer existed → failure on every non-macOS runner.

Fix: Regenerated the 8 deleted Linux snapshots on a matching environment (Linux, R 4.6.0, serocalculator 1.4.0.9013, JAGS 4.3.2 — same as the CI ubuntu runner). They now include the full-dataset fitted/residual rows and the new Stratification column (e.g. Subject,Iso_type,Stratification,t,fitted,residual). The full test-run_mod.R suite passes locally (FAIL 0).

• Diagnose failures (missing Linux snapshots; no live merge error)
• Regenerate Linux (root) snapshots
• Run full test-run_mod.R suite → all pass locally
• Commit & push (63db4f3)

Notes

• No code change to calc_fit_mod.R was needed beyond an incidental trailing-whitespace cleanup; the merge logic already works on R 4.6.0.
• The reviewer's minor suggestions (Ss mcmc diagnostic functions #3/Create README.md #4 — documenting that Stratification = "None" in the unstratified case) are optional polish and not blocking. Let me know if you'd like those roxygen clarifications added.
• ⚠️ Snapshots are sensitive to platform/dependency versions. I generated these on a Linux runner matching CI; the new push will reverify on Windows + all Ubuntu jobs. If any platform still reports a diff, it would be a genuine numeric difference to review.
  • Branch: debug_calcfitmod

[sschildhauer]

@claude please review

[claude]

Claude finished @sschildhauer's task in 1m 48s —— View job

---

Code Review: PR #240 — Debug calcfitmod

• Read CLAUDE.md and repo guidelines (no root CLAUDE.md found)
• Read changed files (R/calc_fit_mod.R, R/Run_Mod.R)
• Review documentation changes (man/*.Rd, NEWS.md, WORDLIST)
• Check CI status for the latest commit (9 ✅ / 1 ❌ → the failure is just the Claude Code Review workflow itself; all tests, lint, R-CMD-check, pkgdown, coverage pass)
• Provide review feedback

---

Summary

The core change is sound and the intent matches the PR description:

Before	After
calc_fit_mod() received dl_sub (filtered to current stratum), producing fitted/residuals only for that subset on each iteration; the last iteration silently overwrote prior strata because fit_res was reassigned, not concatenated.	calc_fit_mod() now receives the full data once, together with strat, and computes fitted/residuals for every observation in a single merge keyed by (Subject, Iso_type[, Stratification]).

The new Stratification column in the output ("None" when unstratified) is well documented and the snapshots reflect it. Nice catch on the underlying bug.

---

Findings

🟢 Correctness — looks good

• R/Run_Mod.R:201-203 — passing data (not dl_sub) and strat is the right fix.
• R/calc_fit_mod.R:50-57 — merge(..., all.y = TRUE) correctly preserves every observation from original_data even if a (Subject, Iso_type, Stratification) tuple has no posterior estimate; those rows surface as NA fitted/residual, which is the expected behavior.
• Snapshot files are consistent across the darwin/ and root variants and now include the Stratification column; nostrat uses "None" exactly as documented.

🟡 Suggestions (non-blocking)

1. strat has no default in calc_fit_mod() (R/calc_fit_mod.R:23-25). Since calc_fit_mod is @keywords internal and only called by run_mod, this is fine — but a default of strat = NA would make the no-stratification branch self-contained and reduce the chance of a future caller forgetting the argument:

   calc_fit_mod <- function(modeled_dat, original_data, strat = NA)

2. NA values in the stratification column are silently dropped from modeling but preserved in original_data. In R/Run_Mod.R:105-107, dplyr::filter(.data[[strat]] == i) drops rows where the strat column is NA. But calc_fit_mod then receives the full data, so those NA-strat rows will land in matched_dat with Stratification = NA and no posterior to merge against → rows with NA fitted/residual will appear in fitted_residuals. If that's intentional (full-data output, NA where un-modeled), great; if not, consider filtering them out or documenting the behavior. Worth at least a sentence in the @returns doc.

3. Potential name collision on "Stratification" (R/calc_fit_mod.R:36). rename("Stratification" = strat) will fail if the input data already has a column literally named Stratification and the user passes a different column via strat. Unlikely in practice, but a dplyr::rename with the target name already present errors out. A one-line guard or an dplyr::any_of cleanup before the rename would harden this.

4. Unrelated WORDLIST change. inst/WORDLIST removes allowlist and adds calc. allowlist is no longer referenced in any spell-checked file (only in .github/workflows/claude.yml as a comment), so removing it is safe — but it's unrelated to this PR's purpose. Not a blocker; just noting for cleanliness.

5. Minor style nits in R/calc_fit_mod.R:

   • Line 26: blank line inside the function body at the very top is unusual.
   • Line 35: trailing whitespace after .data$result, (Fix this →).

6. NEWS wording (NEWS.md:96). Current: "Fixing calc_fit_mod to include all stratifications in fitted_residuals attributes (Debug calcfitmod #240)." This understates the change — it also (a) computes fitted/residuals over the full dataset rather than the last stratum's filtered subset, and (b) adds a Stratification column even in unstratified runs. Consider rewording to something like:

   • run_mod()'s fitted_residuals attribute now covers all observations across all strata (previously only the last stratum was retained) and always includes a Stratification column ("None" when unstratified). (Debug calcfitmod #240)

🟢 Documentation

• @param strat is now in both R/calc_fit_mod.R and man/calc_fit_mod.Rd ✅
• @returns documents the new Stratification field and the "None" default ✅
• man/run_mod.Rd regenerated consistently with the roxygen ✅

🟢 Tests / Snapshots

• All R-CMD-check jobs (ubuntu/macOS/windows) pass on the latest commit (7c41296).
• Both darwin/ and root snapshot variants are present and include the new Stratification column.
• The run_mod.md deparse snapshots updated for the new attribute encoding.

---

Verdict

Approve-with-nits. The fix is correct, well tested, and CI is green on every platform. Items (1)–(3) and (6) are quality-of-life follow-ups; (5) is whitespace. None block merge.
• Branch: debug_calcfitmod

[sschildhauer]

@claude, please implement all suggestions. Keep all edits within in this thread.

[claude]

Claude finished @sschildhauer's task in 6m 33s —— View job

---

Review suggestions implemented — pushed as 97a76da

• Read current state of R/calc_fit_mod.R, R/Run_Mod.R, NEWS.md, inst/WORDLIST
• (1) Default strat = NA — already present in the current branch at R/calc_fit_mod.R:31, no change needed
• (2) Document NA-strat row behavior — added @returns note that rows with NA in the stratification column are retained with NA fitted/residual values; also documented that Stratification = "None" in the unstratified case
• (3) Guard against Stratification name collision — added a conditional rename guarded by if (strat != "Stratification") using dplyr::all_of(strat) so the rename is a no-op when the input already uses the canonical name
• (4) WORDLIST cleanup — restored allowlist (the removal was unrelated to this PR). Kept calc because spell-check tokenizes calc_fit_mod in NEWS.md and flags calc; removing it caused a spell_check_package() failure when I tried it
• (5) Style nits — removed the stray blank line at the top of the function body and cleared all trailing whitespace in R/calc_fit_mod.R
• (6) NEWS wording — rewrote NEWS.md:97 to reflect both fixes:

  run_mod()'s fitted_residuals attribute now covers all observations across all strata (previously only the last stratum was retained) and always includes a Stratification column ("None" when unstratified). (Debug calcfitmod #240)

• Re-ran devtools::document() — man/calc_fit_mod.Rd matches; no unintended drift
• Re-ran spelling::spell_check_package() — No spelling errors found.
• Re-ran full test-run_mod.R suite — 26/26 pass (0 failures, 0 errors, 0 warnings)
• Commit & push (97a76da)

Notes

• The "Suggestion (1)" item in my earlier review was based on a slightly stale view of the function signature — strat = NA was already in place at R/calc_fit_mod.R:31 on the latest commit. No code change was needed there.
• I diverged from the review on WORDLIST in one way: I restored allowlist (the unrelated removal) but kept calc. Empirically, removing calc fails spell check because calc_fit_mod from NEWS.md tokenizes to calc/fit/mod. The calc entry is effectively required by this PR.
• The new name-collision guard is intentionally minimal: a single if around the rename() call using dplyr::all_of(). No change to public-facing behavior.
  • Branch: debug_calcfitmod • Commit: 97a76da

[sschildhauer]

@claude, please review

[claude]

Claude finished @sschildhauer's task in 1m 35s —— View job

---

Code Review: PR #240 — Debug calcfitmod

• Read repo guidelines / CLAUDE.md (none at root)
• Read current state of R/calc_fit_mod.R, R/Run_Mod.R
• Review documentation, snapshots, WORDLIST
• Check CI status for latest commit
• Post final review

---

Summary

The two-part fix lands cleanly:

Before	After
calc_fit_mod() got dl_sub (per-stratum filtered); the last iteration silently overwrote prior strata in fitted_residuals	calc_fit_mod() gets full data + strat, merges once on (Subject, Iso_type[, Stratification])
No Stratification column in fitted_residuals	Stratification always present ("None" when unstratified)

CI is fully green on the latest commit (c4e64f9) — 10/10 workflows passing (R-CMD-check, pkgdown, NEWS check, Version increment, Documentation, Spellcheck, Lint, check-readme, test-coverage, Claude Code Review).

The earlier review nits from this thread have been addressed: strat = NA default present (R/calc_fit_mod.R:31), name-collision guard on "Stratification" rename (R/calc_fit_mod.R:44-47), NA-strat behavior documented (R/calc_fit_mod.R:24-27, man/calc_fit_mod.Rd:33-36), allowlist restored and calc retained in inst/WORDLIST, and the NEWS entry now reflects both fixes (NEWS.md:97).

---

Correctness — looks good

• R/Run_Mod.R:237-239 — calc_fit_mod(modeled_dat = jags_out, original_data = data, strat = strat). Passing the full data (not dl_sub) and strat is the minimal, correct change. The per-stratum filter loop above only governs modeling; fitted/residual computation is now decoupled and runs once over everything.
• R/calc_fit_mod.R:60-68 — merge(..., all.y = TRUE) on the appropriate key preserves every observation, surfacing un-modeled rows as NA fitted/residual. Documented at R/calc_fit_mod.R:24-27 — behavior is intentional.
• R/calc_fit_mod.R:44-47 — if (strat != "Stratification") { rename("Stratification" = all_of(strat)) } correctly handles the edge case where the user's strat column is already named Stratification.
• Snapshots — verified strat-fitted_residuals.csv contains both typhi and paratyphi, and sim-strat-fitted_residuals.csv contains both stratum 1 and stratum 2. Row count expanded from 214 → 904 data rows in strat-fitted_residuals.csv, exactly matching the bug being fixed. nostrat snapshots use None as documented. Header order Subject,Iso_type,Stratification,t,fitted,residual matches R/calc_fit_mod.R:75-76. Both darwin/ and root snapshot variants exist with identical schemas; numeric differences between them are normal platform/JAGS variance.

---

Findings

🟡 Minor doc inconsistency — None vs "None" (non-blocking, carried over from prior review)

R/Run_Mod.R:54 and R/Run_Mod.R:87 still write `None` (unquoted), while R/calc_fit_mod.R:17 correctly writes `"None"` (quoted). Since the actual R character value is "None", the quoted form is more precise. Same applies to the generated man/run_mod.Rd:129 and man/run_mod.Rd:170.

- - The default value for no stratification is `None`.
+ - The default value for no stratification is `"None"`.

- stratification will be `None`.
+ stratification will be `"None"`.

Fix this →

🟢 Documentation

• @param strat present in both R/calc_fit_mod.R:10-11 and man/calc_fit_mod.Rd:15-16 ✅
• @returns documents the new Stratification field, the "None" default, and the NA-strat behavior ✅
• man/run_mod.Rd regenerated consistently ✅

🟢 Tests / Snapshots

• All snapshot files include the new Stratification column.
• Per-stratum coverage verified in both stratified snapshots (no longer last-stratum-only).
• Both darwin/ and root variants present with consistent schemas; numeric differences are platform-driven and expected.

🟢 NEWS.md (NEWS.md:97)

run_mod()'s fitted_residuals attribute now covers all observations across all strata (previously only the last stratum was retained) and always includes a Stratification column ("None" when unstratified). (#240)

Accurate and captures both behavioral changes.

🟢 WORDLIST

allowlist restored, calc retained (required because spell-check tokenizes calc_fit_mod in NEWS.md). Both entries warranted.

---

Verdict

Approve-with-nit. The fix is correct, well-tested, and CI is green across all platforms. The only remaining item is the `None` → `"None"` doc-quoting harmonization in R/Run_Mod.R:54 and R/Run_Mod.R:87 (non-blocking, polish-level). Nothing blocks merge.

---

• Branch: debug_calcfitmod

[sschildhauer]

@claude, please make the above recommended changes.

[claude]

Claude finished @sschildhauer's task in 1m 18s —— View job

---

Implemented review nit — pushed as a15dd19

• Read R/Run_Mod.R to confirm current state of lines 54 and 87
• Update `None` → `"None"` in R/Run_Mod.R:54 and R/Run_Mod.R:87
• Regenerate man/run_mod.Rd via devtools::document() (only the two corresponding lines changed)
• Commit and push (a15dd19)

Diff

--- a/R/Run_Mod.R
+++ b/R/Run_Mod.R
@@ -54  -    The default value for no stratification is `None`.
+    The default value for no stratification is `"None"`.
@@ -87  -  stratification will be `None`.
+  stratification will be `"None"`.

man/run_mod.Rd regenerated cleanly — only the two matching lines updated (no unintended drift).

---

• Branch: debug_calcfitmod • Commit: a15dd19

[sschildhauer]

@claude, please review

[claude]

Claude finished @sschildhauer's task in 1m 31s —— View job

---

Code Review: PR #240 — Debug calcfitmod

• Read repo guidelines / CLAUDE.md (none at root)
• Read current state of R/calc_fit_mod.R, R/Run_Mod.R
• Verify the `None` → `"None"` doc-quoting fix landed correctly
• Check CI status for latest commit
• Post final review

---

Summary

CI is fully green on a15dd19 — 9/9 substantive workflows passing (R-CMD-check, pkgdown, NEWS check, Version increment, Documentation, Spellcheck, Lint, check-readme, test-coverage). The one "failure" is the Claude Code Review workflow itself, not a code check.

The last review nit was implemented cleanly:

Location	Before	After
R/Run_Mod.R:54	`None`	`"None"` ✅
R/Run_Mod.R:87	`None`	`"None"` ✅
man/run_mod.Rd:129,170	\code{None}	\code{"None"} ✅ (regenerated)

All three doc files (R/Run_Mod.R, R/calc_fit_mod.R:17, man/calc_fit_mod.Rd:25) now use the same quoted form.

---

Correctness — looks good

• R/Run_Mod.R:237-239 — calc_fit_mod(modeled_dat = jags_out, original_data = data, strat = strat) passes the full data (not the per-stratum filtered dl_sub), so fitted/residuals now cover every observation in a single merge. Correct.
• R/calc_fit_mod.R:60-68 — merge(..., all.y = TRUE) on (Subject, Iso_type) or (Subject, Iso_type, Stratification) preserves every observation, surfacing un-modeled rows with NA fitted/residual. Documented at R/calc_fit_mod.R:24-27.
• R/calc_fit_mod.R:44-47 — the if (strat != "Stratification") guard correctly handles the edge case where the user's strat column is already named Stratification.
• Snapshots verified:
  • strat-fitted_residuals.csv contains both typhi and paratyphi (no longer last-stratum-only).
  • sim-strat-fitted_residuals.csv contains both stratum 1 and stratum 2.
  • nostrat-fitted_residuals.csv uses None as documented.
  • All snapshots have header Subject,Iso_type,Stratification,t,fitted,residual, matching R/calc_fit_mod.R:75-76.
  • darwin/ and root variants are present with identical schemas and row counts (905, 905, 1134) — numeric differences are normal platform/JAGS variance.

Documentation — looks good

• @param strat present in both R/calc_fit_mod.R:10-11 and man/calc_fit_mod.Rd ✅
• @returns documents the Stratification field, the "None" default, and the NA-strat behavior ✅
• man/run_mod.Rd regenerated consistently with the roxygen ✅
• NEWS entry (NEWS.md:97) accurately captures both behavioral changes ✅
• inst/WORDLIST: allowlist restored, calc retained ✅

Findings

No outstanding blockers. No new findings beyond what was already addressed in earlier iterations of this thread.

---

Verdict

Approve. The fix is correct, well-tested, CI is green across all platforms, and every prior review item — including the final `None` → `"None"` quoting nit — has been addressed. Ready to merge.

---

• Branch: debug_calcfitmod

[sschildhauer]

@Kwan-Jenny, This should be ready for review