Decay curve 2 - #392
Conversation
Codecov Report❌ Patch coverage is
|
|
@copilot rebase on main; resolve merge conflicts |
|
@d-morrison I've opened a new pull request, #486, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: d-morrison <2474437+d-morrison@users.noreply.github.com>
Co-authored-by: d-morrison <2474437+d-morrison@users.noreply.github.com>
Co-authored-by: d-morrison <2474437+d-morrison@users.noreply.github.com>
Co-authored-by: d-morrison <2474437+d-morrison@users.noreply.github.com>
ARD summary — round pushed as
|
| # | Finding | Disposition | What changed |
|---|---|---|---|
| 1 | NEWS.md:74-75 — bullet claims a spurious and entry was removed from Imports; false on every count |
Address | Bullet deleted. and is real (and::or() at R/check_strata.R:45) and still in Imports; the diff only reordered it. |
| 2 | Missing NEWS bullet for the cli::cli_abort() change |
Address | Added, scoped to autoplot.seroincidence() only — see the correction below. |
| 3 | codecov/patch 33.68% vs 84.80% target |
Defer | Tracked in #560 per your call; the Shiny UI/reactive code is inherently hard to unit-test and the decay-curve math is covered. |
| 4 | lint-changed-files red (7 indentation_linter findings) |
Address | See below. |
| 5 | Branch conflicted with main (mergeable_state: dirty) |
Address | See below. |
Correction to the PR description
The description claims the cli::cli_abort() improvement covers both autoplot.seroincidence() and autoplot.seroincidence.by(). Only the first is true — this PR's sole change to R/autoplot.seroincidence.by.R is a brace-style fix (now reverted, see below). The NEWS bullet is worded to match what the diff actually does.
That file's own cli::cli_abort() call, which is main's content and not this PR's, turns out to be broken: it passes two positional strings the way stop() would, but cli_abort() forwards its ... to rlang::abort(), whose second positional parameter is class — so the half of the message naming build_graph never renders and a sentence becomes the condition class. Filed as #606 rather than fixed here, since the fix belongs on main.
Merge with main
main had moved 164 commits ahead, including the pkgdown → altdoc migration (#504) that deleted pkgdown/_pkgdown.yml. Conflicts resolved as:
DESCRIPTION— tookmain's version line, bumped to1.4.1.9016soversion-checkstill sees the branch ahead.pkgdown/_pkgdown.yml— accepted the deletion, and moved this PR's six new reference entries into the replacement index,altdoc/reference.qmd, under the same "Visualize data" section they occupied. Taking the deletion alone would have quietly dropped the new functions off the documentation site.
Lint
#558 has since been resolved: it wasn't a lintr regression, it's that the package uses the 4-space double indent the tidyverse style guide dropped, and the decision was to migrate incrementally, with #392's own new files named as a follow-up. So:
- Reindented the six new decay-curve signatures to the 2-space single indent, with
) {on its own line.man/needs no regeneration — roxygen already emits that shape in\usage{}regardless of source formatting. - Reverted four out-of-scope drive-by edits (
R/autoplot.seroincidence.by.R,R/plot_curve_params_one_ab.R,R/stratify_data.R,R/summary.seroincidence.R). They were comment removals and a brace fix, and one deleted a genuinely useful reference — the delta-method citation next toSE = se_log_lambda * incidence.rate. Reverting also takes those legacy files out of the changed-file set, so their untouched 4-space code stops being linted, which is exactly the incremental approach Migrate to tidyverse 2-space function-arg indentation incrementally (lintr 3.4.0) #558 settled on.
A local lintr::lint_package() over the PR's full changed-file set now reports zero lints.
Two more things found on the way
lint-changed-filesonly lints the first 30 changed files of a PR #608 —lint-changed-filesreads only the first 30 files of a PR's file list (gh::gh()doesn't paginate by default), so on this 38-file PR it silently skipped 8 files. It missed two realline_length_linterfindings in this PR's own new test file; both are fixed in this push.lint-changed-lines(Add lint-changed-lines workflow (incremental linting) — supersedes #559 #564) computes its set from the git diff and has no such cutoff..gitignoreignoresaltdoc/, which is a tracked source directory #607 —.gitignoreignoresaltdoc/, a tracked source directory, so adding a page there needsgit add -f. Hit while editingaltdoc/reference.qmd.- Eight internal helpers carry both
@noRdand@keywords internal;@noRdwins and suppresses their docs #609 — eight internal helpers (including this PR's.validate_decay_params()) carry both@noRdand@keywords internal;@noRdwins and suppresses their docs entirely. Repo-wide, so filed rather than folded in here.
Generated by Claude Code
main advanced 164 commits since this branch last synced, including the pkgdown -> altdoc documentation migration (#504) that deleted `pkgdown/_pkgdown.yml`. Conflicts resolved as: - `DESCRIPTION`: keep main's version line and bump to 1.4.1.9016, so `version-check` still sees the branch ahead of main. - `pkgdown/_pkgdown.yml`: accept main's deletion, and move this PR's six new reference entries (`antibody_decay_curve()`, `pathogen_decay_curve()`, `plot_decay_curve()`, `curve_app()`, `t1f()`, `y1f()`) into the replacement index, `altdoc/reference.qmd`, under "Visualize data" -- the section they occupied in the pkgdown config. Dropping them with the file would have left the new functions off the documentation site. Also applies the two NEWS.md fixes from the last review round: delete the bullet claiming a spurious `and` entry was removed from `Imports` (`and` is a real dependency, called at `R/check_strata.R:45`, and was never removed), and add the missing bullet for the `cli::cli_abort()` change in `autoplot.seroincidence()`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JCwW1H9AFVc8J1eRcobG32
Two changes, both aimed at getting `lint-changed-files` green without reformatting code this PR has no business touching. Reindent the six new decay-curve function signatures from the package's older 4-space double indent to the 2-space single indent lintr 3.4.0's `indentation_linter` now requires, closing the "reindent new/edited function defs as PRs touch them" follow-up #558 assigned to this PR. Closing `) {` moves to its own line, matching what roxygen already emits in the generated `\usage{}` blocks, so `man/` needs no regeneration. Revert four out-of-scope drive-by edits to files this PR otherwise has no reason to change: `R/autoplot.seroincidence.by.R`, `R/plot_curve_params_one_ab.R`, `R/stratify_data.R`, and `R/summary.seroincidence.R`. These were comment removals and a brace-style fix, and one of them deleted a useful citation (the delta-method reference next to `SE = se_log_lambda * incidence.rate`). Reverting them also takes those four legacy files out of the changed-file set, so `lint-changed-files` no longer lints their untouched 4-space code -- the incremental-migration approach #558 settled on. Also wraps two over-length lines in the new `tests/testthat/test-validate_decay_params.R`. CI never flagged these because `lint-changed-files` reads only the first 30 files of the PR's file list (#608); a local `lint_package()` over the full changed-file set now reports zero lints. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JCwW1H9AFVc8J1eRcobG32
6662946 to
c66bb77
Compare
`lint-changed-lines` flags `app.R:5`'s `library(serocalculator)` under `undesirable_function_linter`, which the repo's `.lintr.R` configures to steer callers toward `::` rather than modifying the global search path. The call was already redundant: the next line invokes `serocalculator::curve_app()` namespace-qualified, so nothing depended on the package being attached. Deployment is unaffected too --- rsconnect discovers dependencies via `renv::dependencies()`, which detects `pkg::fn()` usage as well as `library(pkg)`. `lint-changed-files` never caught this because `lintr::lint_package()` scans `R/`, `tests/`, `inst/`, `vignettes/`, `data-raw/`, and `demo/`, not repository-root scripts. `lint-changed-lines` works from the diff, so it sees `app.R` --- an independent reason to prefer it over the file-based check, on top of the 30-file pagination cutoff in #608. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JCwW1H9AFVc8J1eRcobG32
|
Worth noting why this one only surfaced now, since it's a second independent gap in (The two commits before this were also force-pushed once, to correct their author identity. Trees and parentage are byte-identical to what was reviewed above — no content changed.) Generated by Claude Code |
Codecov flags 187 uncovered lines in the diff. 178 of them are the Shiny app (`curve_app_ui`, `curve_app_server`, `curve_app`), deferred to #560 per maintainer direction. The remaining 9 are not Shiny code and are worth covering here: `R/autoplot.seroincidence.R` (8 lines) is the `cli::cli_abort()` branch this PR added, which had no test at all --- so the NEWS bullet describing the new message was unverified by anything. The new test asserts the rendered error actually contains `build_graph`, rather than merely that it errors: a first-clause-only assertion would still pass under the `cli_abort()` misuse in #606, where the half of the message naming the fix is silently dropped. A companion case covers the success path and `log_x = TRUE`. `R/antibody_decay_function.R` (1 line) is the `rho < 1` guard. Its sibling `alpha < 0` guard was already tested; this one was not. Both files are lint-clean, but the container running this has no testthat or package dependencies installed, so CI is their first execution. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JCwW1H9AFVc8J1eRcobG32
Codecov round —
|
| File | Missing | Disposition |
|---|---|---|
R/curve_app_ui.R |
94 | Defer — #560 |
R/curve_app_server.R |
80 | Defer — #560 |
R/curve_app.R |
4 | Defer — #560 |
R/autoplot.seroincidence.R |
8 | Address |
R/antibody_decay_function.R |
1 | Address |
178 of the 187 are the Shiny app, which is exactly what #560 defers per your earlier call — unchanged, no action.
The other 9 aren't Shiny code and aren't in #560's scope:
R/autoplot.seroincidence.R(22.22%, -8.55%) — these 8 lines are thecli::cli_abort()branch this PR added. It had no test at all, which also means the NEWS bullet I added last round described behavior nothing verified. Addedtests/testthat/test-autoplot.seroincidence.R. It asserts the rendered message actually containsbuild_graph, not merely that the call errors — deliberately, because a first-clause-only assertion would still pass under thecli_abort()misuse inautoplot.seroincidence.by()'s "graphs cannot be extracted" error drops half its message #606, where the half of the message naming the fix is silently dropped. A companion case covers the success path andlog_x = TRUE.R/antibody_decay_function.R(97.05%) — the one missing line is therho < 1guard. Its siblingalpha < 0guard was already tested; this one wasn't.
Both files are lint-clean, but this session's container has no testthat or package dependencies installed, so CI is their first actual execution — worth a look at the R-CMD-check results rather than taking my word for it.
Note this push cancels the claude-review run that was in flight for c254aba. That run was reviewing a diff these tests have since changed, so its verdict would have been stale either way; the re-run against 17c7496 reviews the real thing.
Also green since the last round
lint / lint-changed-lines, lint-changed-files, docs-check, Spellcheck, version-check, test-coverage, check / Check Changelog Action, and R-CMD-check on macOS and ubuntu-release all passed on c254aba. docs-check passing is the one that confirms the 2-space signature reindent didn't desync man/.
Generated by Claude Code
The altdoc site keeps its reference topics in two independent places: `altdoc/reference.qmd` (the "Package index" page) and the `Reference` section of `altdoc/quarto_website.yml` (the sidebar navigation). Porting this PR's six new topics off the deleted `pkgdown/_pkgdown.yml` updated only the first, which would have listed them on the index page while leaving them absent from the sidebar. Adds all six to the sidebar's "Visualize data" section, matching their placement on the index page and the section they occupied in the old pkgdown config. A mechanical cross-check of the two lists now shows 42 topics each, with an empty symmetric difference and a backing `man/*.Rd` for every entry. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JCwW1H9AFVc8J1eRcobG32
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Code reviewRe-reviewed the current diff (head Four parallel review passes (2× CLAUDE.md compliance, 2× bug/logic scan) turned up one new, validated finding — posted inline:
A second candidate finding (missing test coverage for the new No bugs, security issues, or logic errors were found in the introduced decay-curve math ( VerdictNeeds more work — one file-naming convention issue to address before merge; everything else checks out. |
|
Claude finished review — View run gh pr comment 392 --repo UCD-SERG/serocalculator --body "$(cat <<'EOF' Code reviewRe-reviewed the current diff (head Four parallel review passes (2× CLAUDE.md compliance, 2× bug/logic scan) turned up one new, validated finding — posted inline:
A second candidate finding (missing test coverage for the new No bugs, security issues, or logic errors were found in the introduced decay-curve math ( VerdictNeeds more work — one file-naming convention issue to address before merge; everything else checks out. |
`.github/copilot-instructions.md` (the source of truth root `CLAUDE.md` defers to) marks "File name should match the function name" as CRITICAL for all new code. Four files this PR adds broke it: R/antibody_decay_function.R -> R/antibody_decay_curve.R R/antigen_decay_curve.R -> R/pathogen_decay_curve.R R/t1.R -> R/t1f.R R/y1.R -> R/y1f.R The `antigen_decay_curve.R` / `pathogen_decay_curve()` pair was the worst of the four: the file and the function named different biological entities, so grepping for either name found only half the story. Test files are renamed to match, keeping testthat's `test-<source>.R` pairing intact. Each affected `man/*.Rd` gets its `% Please edit documentation in R/<file>.R` header updated to the new path, which is what `devtools::document()` would have written; no other generated content changes, since roxygen derives everything else from the source rather than the filename. `DESCRIPTION` has no `Collate` field, and neither the renamed sources nor their tests are referenced by path anywhere else in the repo, so nothing else needed updating. Verified mechanically rather than by eye: every `R/` file this PR touches now defines a top-level function whose name matches its filename, with `.validate_decay_params()` in `validate_decay_params.R` following the documented internal-helper convention. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JCwW1H9AFVc8J1eRcobG32
ARD summary — round pushed as
|
| # | Finding | Source | Disposition |
|---|---|---|---|
| 1 | Four new files' names don't match the function they define | claude-review (inline) |
Address — d785029 |
| 2 | Shiny app lacks test coverage | claude-review (considered, not raised) |
Defer — #560, already tracked |
| 3 | Six new topics missing from the docs sidebar | self-review | Address — 4defddc |
1. File naming — Addressed
Verified and correct; renames in d785029, details in the inline reply. Thread resolved.
2. Shiny coverage — Defer
The review raised and self-dismissed this one; agreed, and it's already tracked in #560. Worth noting the coverage picture improved this round: R/autoplot.seroincidence.R went 22.22% → 100% (+69.23%) and R/antibody_decay_function.R 97.05% → 100%, so all 178 remaining uncovered lines are now the three Shiny files and nothing else.
3. Sidebar — Addressed (found by self-review, not the bot)
The altdoc site keeps reference topics in two independent hand-maintained lists: altdoc/reference.qmd (the index page) and the Reference section of altdoc/quarto_website.yml (the sidebar). Porting this PR's six new entries off the deleted pkgdown/_pkgdown.yml updated only the first.
Confirmed against the actual deployed preview rather than by reading the config — reading pr-preview/pr-392/reference.html off the gh-pages branch and counting occurrences:
| Topic | Occurrences |
|---|---|
autoplot.pop_data, graph_loglik, log_likelihood, est_seroincidence_by (established) |
3 each |
| the six new decay-curve topics | 2 each |
Established topics appear three times — sidebar entry, index link, index text. The six new ones appeared twice: index only. Cross-checked on the link itself (man/log_likelihood.html twice, man/antibody_decay_curve.html once).
So this would have shipped six functions listed on the package index but unreachable from site navigation, with both lists rendering cleanly and no check catching it. Fixed in 4defddc; the two lists now agree at 42 topics each with an empty symmetric difference and a backing man/*.Rd for every entry. The next preview build should show 3 occurrences each — checkable the same way.
Filed #610 proposing this cross-check as CI, since it's three assertions over two files and passes on main today.
Also filed this session
#606 (cli_abort() misuse in autoplot.seroincidence.by() drops half its message — main's code, not this PR's), #607 (.gitignore shadows the tracked altdoc/ directory), #608 (lint-changed-files only reads the first 30 changed files), #609 (eight helpers carry both @noRd and @keywords internal).
Generated by Claude Code
|
Claude finished review — View run Code Review Summary — PR #392 (Decay curve 2)This is a confirming review; no Prior verdict: The most recent Verification against the current diff (head
No new issues found. VerdictReady for merge — the previously-flagged file-naming violation is confirmed fixed, all earlier-round findings remain resolved, and no new issues were introduced by this round's changes (file renames + doc-sidebar sync). |
Picks up #604/#605 (README docs-link fix and its NEWS wording follow-up). main reached 1.4.1.9016, the same version this branch carried, so `version-check` -- which requires the branch to exceed main -- would have started failing despite the merge itself being conflict-free. Bumped to 1.4.1.9017 in the same commit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JCwW1H9AFVc8J1eRcobG32
Round complete — clean review, plus one late catchThe review at
One more fix, pushed as
|
| Topic | Pre-fix | Post-fix |
|---|---|---|
| all six new decay-curve topics | 2 | 3 ✅ |
log_likelihood, graph_loglik, autoplot.pop_data (controls) |
3 | 3 |
Three occurrences = sidebar entry + index link + index text, matching every established topic. File size changed (71,540 → 72,784 bytes), confirming a genuinely new build rather than a cached read. So the six functions are now reachable from site navigation, verified in the rendered output rather than only in the config.
Status
CI is re-running against 1d670de. Once it goes green this PR is ready to merge, with codecov/patch / codecov/project red solely on the #560 Shiny deferral — every non-Shiny line in the diff is at 100%.
Not merging; that's your call.
Generated by Claude Code
Code reviewRe-reviewed the current diff (head The only change since that review is commit
No new issues found. VerdictReady for merge |
|
Claude finished review — View run Posted the confirming review: #392 (comment) Summary: The only change since the last full review ( VerdictReady for merge |
|
Ready for merge —
|
| ✅ Passed (19) |
|---|
R-CMD-check ×5 (macOS, Windows, ubuntu release / devel / oldrel-1) · test-coverage · docs-check · docs / docs · version-check · lint / lint-changed-lines · lint-changed-files · Spellcheck · check / Check Changelog Action · CodeFactor · review / claude-review · review / require-review · review / gather-context |
codecov/patch (36.20%), codecov/project (80.98%) — the Shiny gap tracked in #560, deferred on your call. All 178 uncovered lines are curve_app_ui.R / curve_app_server.R / curve_app.R; every non-Shiny line in the diff is at 100%. |
Review: claude-review returned Ready for merge at this head — verdict posted and its check run completed, with require-review green. Copilot has not re-reviewed since January; the clean call rests on claude-review's verdict.
Mergeability, re-verified just now: main ahead by 0, zero conflicts, branch at 1.4.1.9017 vs main's 1.4.1.9016.
What this round fixed
- Merged 164 commits of
maindrift, including the pkgdown → altdoc migration that deletedpkgdown/_pkgdown.yml; moved the six new reference entries intoaltdoc/reference.qmd. - Deleted the false NEWS bullet about
andbeing removed fromImports; added the missingcli::cli_abort()bullet, scoped to what the diff actually changes. - Reindented six new function signatures to 2-space per Migrate to tidyverse 2-space function-arg indentation incrementally (lintr 3.4.0) #558's incremental migration, and reverted four out-of-scope drive-by edits (one of which had deleted the delta-method citation in
summary.seroincidence()). - Removed the redundant
library()call fromapp.R. - Covered the two non-Shiny uncovered branches:
autoplot.seroincidence()22.22% → 100%,antibody_decay_curve()'srho < 1guard → 100%. - Renamed four files to match their exported functions, per the review's finding.
- Added the six new topics to the docs sidebar, which the reference-index port had missed — verified in the rendered preview (2 → 3 occurrences each, controls unchanged).
- Merged
mainagain at the end and bumped the version, catching aversion-checkparity failure that would otherwise have surfaced after this PR was reported ready.
Issues filed along the way
#606 (cli_abort() misuse drops half its message — main's code) · #607 (.gitignore shadows tracked altdoc/) · #608 (lint-changed-files reads only the first 30 changed files) · #609 (eight helpers' docs suppressed by @noRd) · #610 (CI cross-check for the two reference lists) · d-morrison/gha#312 (review workflow posted its own gh command instead of the review body)
Not merging — that's your call. Done here; PR is free.
Generated by Claude Code
This pull request introduces a new interactive Shiny application for visualizing antigen-antibody kinetics models, along with several supporting functions and package infrastructure updates. It also adds new plotting and mathematical functions for decay curves, exposes additional functions to users, and improves error messaging. The changes enhance the package's interactivity, usability, and extensibility for modeling and visualization.
Major feature: Interactive Shiny app for kinetics modeling
curve_app) for interactively exploring antigen-antibody kinetics, with UI (curve_app_ui), server logic (curve_app_server), and a new launch script (app.R). The app allows users to adjust model parameters via sliders and view real-time plots and derived values. [1] [2] [3] [4]New modeling and plotting utilities
antibody_decay_curveandpathogen_decay_curvefunctions for simulating and plotting antibody and pathogen decay over time. [1] [2]plot_decay_curvefor easy plotting of decay functions using ggplot2.t1f(time to end of active infection) andy1f(peak antibody concentration). [1] [2]Package infrastructure and configuration
.Rbuildignoreand added.rscignoreto exclude app and deployment files from builds; incremented package version to 1.4.0.9004. [1] [2] [3] [4]app_config.Rfor app-specific configuration and file access helpers.Error handling improvements
autoplot.seroincidenceandautoplot.seroincidence.byto usecli::cli_abortfor clearer and more informative errors. [1] [2]Miscellaneous
_disable_autoload.R).These changes significantly expand the package's capabilities for modeling, visualization, and interactive exploration of serological data.