Standardise S3 interface for estimate_secondary and update parameter extraction infrastructure

[sbfnk]

Description

This PR closes #1142.

This PR completes the S3 standardisation for estimate_secondary(). Doing so necessitated an update to the infrastructure for extracting delays and parameters.

Changes by section

1. S3 interface standardisation (commits 141dcd1..3f35449)

• Standardise estimate_secondary() return structure to match estimate_infections() with fit, args, and observations elements
• Add S3 methods: get_samples(), get_predictions(), summary(), and plot()
• Create independent forecast_secondary S3 class with dedicated methods
• Add backward compatibility via $ operator for the previous return structure

2. Parameter extraction infrastructure (commits 134691c..5271cf1)

• Implement extract_parameters() for systematic extraction of all model parameters
• Implement extract_delays() with delay name lookup system
• Refactor estimate_secondary() to use modern extraction pattern

3. Delay ID naming system (commits 180fe5e..c684a8b)

• Introduce delay_id_* variables with semantic names (e.g., delay_id_reporting, delay_id_incubation)
• Update all Stan models to use semantic delay IDs
• Add comprehensive tests for delay ID system
• Fix critical bug where wrong delay ID was used in observation model

Note on scope

Whilst the delay ID naming system (delay_id_reporting, etc.) may appear to extend beyond S3 standardisation, it is a necessary prerequisite. The S3 interface requires get_samples() to return interpretable delay parameters. Without semantic delay IDs, delays can only be extracted as delay_params[1], delay_params[2], etc., making it impossible for users to know which delay is which. Semantic IDs enable extract_delays() to return delays with meaningful names that users can interpret and use.

Initial submission checklist

• My PR is based on a package issue and I have explicitly linked it.
• I have tested my changes locally (using devtools::test() and devtools::check()).
• I have added or updated unit tests where necessary.
• I have updated the documentation if required and rebuilt docs if yes (using devtools::document()).
• I have followed the established coding standards (and checked using lintr::lint_package()).
• I have added a news item linked to this PR.

After the initial Pull Request

• I have reviewed Checks for this PR and addressed any issues as far as I am able.

[coderabbitai]

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

This pull request introduces S3 class structures for estimate_secondary and forecast_secondary, refactors delay identifiers throughout the codebase for semantic clarity, and implements accessor methods (get_predictions, get_samples, summary) to provide consistent interfaces across inference classes.

Changes

Cohort / File(s)	Summary
Documentation and Configuration CLAUDE.md	Added workflow guidance for pull requests, including overview, distinctions, and template requirements.
Public API Registration NAMESPACE	Registered multiple S3 method bindings for estimate_secondary, forecast_secondary, estimate_infections, and forecast_infections; exported get_predictions as public function.
Release Notes NEWS.md	Documented new S3 classes and accessor interfaces for estimate_secondary and forecast_secondary, breaking changes to return structures, and new argument CrIs for plot methods.
Parameter Extraction Refactoring R/extract.R	Replaced single-parameter extractor with bulk extract_parameters(samples) and introduced extract_delays(samples) for unified multi-parameter extraction with reverse ID lookup.
Accessor Method Implementations R/get.R	Added comprehensive get_samples.* and get_predictions.* methods for estimate_secondary, forecast_secondary, and estimate_infections; ensured data.table copies to prevent unintended mutations.
Estimate Secondary Refactoring R/estimate_secondary.R	Changed return structure to standardised S3 object with fit, args, observations; introduced deprecation-aware $.estimate_secondary accessor; updated forecast_secondary to independent class; modified prediction access patterns to use get_predictions().
Summary Method R/summarise.R	Added summary.estimate_secondary() with type filtering ("compact"/"parameters") and credible interval support.
Format and Sample Processing R/format.R	Replaced per-parameter extraction with bulk pre-extraction via extract_parameters() and extract_delays(); streamlined delay and parameter handling.
Estimate Functions Parameter Alignment R/estimate_infections.R, R/estimate_truncation.R, R/simulate_infections.R, R/simulate_secondary.R	Updated create_stan_delays argument names from gt/delay/trunc to generation_time/reporting/truncation for semantic clarity.
Delay Creation Logic R/create.R	Introduced delay-specific identifier mapping with delay_id_* variables named by delay type (e.g., delay_id_generation_time, delay_id_reporting).
Check Functions R/checks.R	Updated truncation checks to use delay_id_truncation instead of trunc_id.
Global Variables R/utilities.R	Added "parameter" to global variable declarations for static analysis.
Stan Model Data Definitions inst/stan/data/observation_model.stan, inst/stan/data/rt.stan, inst/stan/data/simulation_*.stan	Renamed delay identifiers: trunc_id → delay_id_truncation, gt_id → delay_id_generation_time, delay_id → delay_id_reporting across all simulation and observation models.
Stan Model Implementations inst/stan/estimate_*.stan, inst/stan/simulate_*.stan	Updated delay PMF computations to use renamed delay identifiers; refactored generation-time, reporting, and truncation delay paths to reference delay_id_generation_time, delay_id_reporting, delay_id_truncation respectively.
Development Tooling inst/dev/stan-to-R.R	Updated delay ID references in generated R extraction code to align with new naming scheme.
Manual Pages man/*.Rd	Added documentation for new methods (plot.forecast_secondary, summary.estimate_secondary, get_predictions, get_samples.*, $.estimate_secondary); updated accessor descriptions for estimate_secondary and estimate_infections; added documentation for new extraction functions (extract_delays, extract_parameters); minor title capitalisations.
Tests tests/testthat/test-*.R	Updated test expectations to reflect new return structures and accessor usage; replaced direct list component access with get_samples() and get_predictions(); added validation for new S3 classes and delay identifier creation; expanded test-delays.R with comprehensive coverage of extract_delays() and delay ID sequencing.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant estimate_secondary
    participant Accessors as get_samples<br/>get_predictions<br/>summary
    participant Stan as Stan Fit

    User->>estimate_secondary: estimate_secondary(...)
    estimate_secondary->>Stan: Stan inference
    Stan-->>estimate_secondary: posterior samples
    estimate_secondary->>estimate_secondary: Assemble fit, args, observations
    estimate_secondary-->>User: S3 object c("epinowfit", "estimate_secondary")

    User->>Accessors: get_samples(object)
    Accessors->>Stan: Extract raw samples
    Accessors-->>User: Formatted data.table
    
    User->>Accessors: get_predictions(object)
    Accessors->>Stan: Derive from sim_secondary
    Accessors-->>User: Predictions with CrIs
    
    User->>Accessors: summary(object, type="compact")
    Accessors->>Accessors: Filter to key parameters
    Accessors-->>User: Summary statistics

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Areas requiring extra attention:

• Accessor method implementations (R/get.R): Multiple get_samples.* and get_predictions.* methods with intricate data assembly logic, date merging, and summarisation; verify correctness of sample extraction from Stan fit and alignment with date columns across all class variants.
• Return structure changes (R/estimate_secondary.R): Critical refactoring from composite list (predictions, posterior, data, fit) to standardised (fit, args, observations); verify deprecation wrapper $.estimate_secondary provides correct backward-compatible access and that all downstream code paths reference get_predictions() consistently.
• Delay ID refactoring consistency: Widespread renaming across Stan models and R checks; cross-reference Stan data declarations with corresponding uses in R functions (create_stan_delays, create_stan_data) to ensure identifiers are passed and indexed correctly throughout.
• Parameter extraction replacement (R/extract.R, R/format.R): Transition from single-parameter extract_parameter() to bulk extract_parameters() and new extract_delays(); verify reverse ID lookup logic, NULL handling, and fallback naming conventions in both extraction and formatting paths.
• Test coverage updates (tests/testthat/): Extensive test rewrites using new accessors; validate that all tested scenarios (NA handling, filtering, accumulation) work correctly with new get_samples() and get_predictions() patterns across both base R and CmdStanR backends.
• Stan model delay path refactoring (inst/stan/estimate_*.stan): Changes to PMF computation order and identifier usage; cross-check that generation-time, reporting, and truncation paths route through correct delay type indices and that resulting observations are correctly processed.

Possibly related PRs

• Extend S3 class structure to forecast_infections() #1156: Introduces independent S3 class for forecast_infections with dedicated get_samples(), summary(), and plot() methods including CrIs argument support, directly paralleling the forecast_secondary changes.
• Standardise S3 interface for estimate_infections and add get_samples() #1144: Expands S3 accessor-based interface across estimate_* classes with get_samples() and get_predictions(), providing foundational changes that this PR builds upon for estimate_secondary.
• Rename internal extraction functions for clarity #1177: Refactors extraction helpers in R/extract.R and call sites in R/format.R, with overlapping logic on parameter extraction function replacement and multi-parameter handling.

Suggested reviewers

• seabbs

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Out of Scope Changes check	❓ Inconclusive	The PR introduces changes to delay ID naming (delay_id_reporting, delay_id_truncation, delay_id_generation_time) across Stan models and R code, which extends beyond the linked issue scope but directly enables the S3 interface standardisation.	Clarify whether delay ID naming refactoring is a necessary prerequisite for S3 standardisation or a separate feature that should be addressed independently.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarises the primary changes: standardising the S3 interface for estimate_secondary and updating parameter extraction infrastructure, reflecting the main objectives.
Linked Issues check	✅ Passed	The PR comprehensively addresses all objectives from issue #1142: standardised S3 interface, get_predictions/get_samples accessors, observations field, fit element, and backward compatibility via $ operator.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (8)

R/create.R (1)

688-705: Delay ID mapping looks correct; consider guarding for unnamed delays

The new delay_id_* construction is consistent with delay_types_groups (one ID per non-empty delay type, used as an index into delay_types_groups), and the concatenation into ret matches the new Stan data fields.

One small robustness improvement: if create_stan_delays() were ever called with unnamed or partially named arguments, delay_names <- names(delays) will yield NULL or empty strings and you would silently generate unnamed or oddly named delay_id_* entries. If you expect all delays to be named (e.g. generation_time, reporting, truncation), you could add a brief check (e.g. stopifnot(!is.null(delay_names), all(nzchar(delay_names)))) to fail fast on misuse. This is optional but would make errors easier to diagnose.

Also applies to: 781-782

man/extract_parameters.Rd (1)

1-19: Consider loosening the samples description beyond rstan::extract()

The documentation for extract_parameters() is clear, but the argument description currently hard-codes samples as coming from rstan::extract(). Given the wider package supports multiple Stan backends, you might want to phrase this more generically (e.g. “a named list of arrays in the format returned by rstan::extract()”) so it remains accurate even if other backends feed into this helper.

R/format.R (1)

226-314: Consider adding guard for seeding_time = 0 as optional defensive programming

The code currently assumes args$seeding_time >= 1. Whilst the estimate_infections code path guarantees this via get_seeding_time() (which enforces max(round(seeding_time), 1)), and format_samples_with_dates() is only called from that path, adding a guard would improve robustness against future code changes:

if (isTRUE(args$seeding_time > 0L)) {
  reported_dates <- dates[-seq_len(args$seeding_time)]
} else {
  reported_dates <- dates
}

Alternatively, use seq_len() with the assumption held, as it handles the boundary more safely than 1:n syntax. The rest of the function is well-structured.

R/get.R (2)

265-316: Guarding assumptions in get_samples.estimate_secondary

The construction of samples via extract_delays(), extract_parameters(), and extract_latent_state("sim_secondary", ...) is well-aligned with the new parameter/delay extraction infrastructure and yields the desired unified samples table.

Two minor robustness points you might consider:

• burn_in and object$observations[(burn_in + 1):.N]$date: at the moment this assumes burn_in < nrow(observations) and burn_in >= 0. This is enforced in estimate_secondary(), but if objects can be constructed or modified elsewhere it may be worth asserting these constraints (or falling back to all dates) here as a defensive check.
• If extract_latent_state("sim_secondary", ...) returns NULL or if the model genuinely has no sim_secondary generated quantity, get_samples() will still work (good), but downstream get_predictions.estimate_secondary() will silently see no sim_secondary rows and return an essentially empty prediction table. A short, explicit error or warning in that situation might make debugging easier for users supplying custom Stan models.

These are not blockers, just potential hardening.

---

324-417: Copy predictions for forecast_ objects to match get_samples behaviour*

The new get_predictions() methods for the fitted objects look good: they consistently derive summaries from reported_cases / sim_secondary samples and merge them with object$observations, and the column ordering matches the expectations in test-estimate_secondary.R.

For the forecast classes, though:

get_predictions.forecast_infections <- function(object, ...) {
  object$predictions
}

get_predictions.forecast_secondary <- function(object, ...) {
  object$predictions
}

these currently return the underlying data.table by reference, while get_samples.forecast_infections() / get_samples.forecast_secondary() return data.table::copy(object$samples). That asymmetry means callers can accidentally mutate object$predictions via the result of get_predictions(), which is surprising given the copying semantics used elsewhere.

Consider aligning these with get_samples.*:

get_predictions.forecast_infections <- function(object, ...) {
-  object$predictions
+  data.table::copy(object$predictions)
}

get_predictions.forecast_secondary <- function(object, ...) {
-  object$predictions
+  data.table::copy(object$predictions)
}

This keeps forecast objects safer from accidental in-place modification, at minimal cost.

tests/testthat/test-estimate_secondary.R (1)

212-241: Consider adding explicit checks for forecast_secondary class and accessors

The forecast_secondary tests still focus on list structure and plotting:

inc_preds <- forecast_secondary(...)
expect_equal(names(inc_preds), c("samples", "forecast", "predictions"))
expect_error(plot(inc_preds, ...), NA)

Given the PR introduces an explicit "forecast_secondary" S3 class and dedicated get_samples() / get_predictions() methods, it would be useful to assert these as well, for example:

expect_s3_class(inc_preds, "forecast_secondary")
expect_true(is.data.frame(get_samples(inc_preds)))
expect_true(is.data.frame(get_predictions(inc_preds)))

This would tighten coverage of the new public surface and catch regressions in S3 registration.

NEWS.md (1)

18-24: Clarify wording around the “breaking change” for estimate_secondary

The NEWS entry states:

Breaking change: The previous return structure with predictions, posterior, and data elements is no longer supported. Use the accessor methods instead.

In the implementation you’ve added a dedicated $.estimate_secondary method that still serves object$predictions, object$posterior, and object$data, albeit with deprecation warnings, to provide a smoother migration path.

To avoid confusion for users reading NEWS, consider softening the wording to something like:

Breaking change: The previous return structure with predictions, posterior, and data elements is deprecated and will be removed in a future release. Use the accessor methods instead (get_predictions(), get_samples(), and object$observations).

Very minor, but it will better reflect the actual behaviour.

If you care about prose style, you could also vary the repeated “Use …” sentence openings in this bullet block, but that’s purely cosmetic.

R/estimate_secondary.R (1)

752-797: Backward-compatible $.estimate_secondary helper is a good migration aid

The custom $ method:

• Intercepts $predictions, $posterior, and $data,
• Emits lifecycle::deprecate_warn() messages pointing users to get_predictions(), get_samples(), and $observations, and
• Falls back to NextMethod("$") for all other element names,

is a sensible way to keep existing user code running while nudging it towards the new accessors.

Two small suggestions:

• It would be worth adding a focused test that exercises each branch ($predictions, $posterior, $data, and a “normal” field like $fit) to ensure the method is registered correctly and that NextMethod("$") still exposes all other list elements as expected.
• In the deprecation messages, you might consider spelling out the full recommended call (e.g. "get_predictions(object)") for absolute clarity.

Functionally, though, this is a solid compatibility layer.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 41adadf and 939c865.

📒 Files selected for processing (44)

• CLAUDE.md (1 hunks)
• NAMESPACE (4 hunks)
• NEWS.md (1 hunks)
• R/checks.R (2 hunks)
• R/create.R (3 hunks)
• R/estimate_infections.R (2 hunks)
• R/estimate_secondary.R (9 hunks)
• R/estimate_truncation.R (1 hunks)
• R/extract.R (4 hunks)
• R/format.R (2 hunks)
• R/get.R (1 hunks)
• R/simulate_infections.R (1 hunks)
• R/simulate_secondary.R (1 hunks)
• R/stanmodels.R (1 hunks)
• R/summarise.R (1 hunks)
• R/utilities.R (1 hunks)
• inst/dev/stan-to-R.R (3 hunks)
• inst/stan/data/observation_model.stan (1 hunks)
• inst/stan/data/rt.stan (1 hunks)
• inst/stan/data/simulation_delays.stan (1 hunks)
• inst/stan/data/simulation_observation_model.stan (1 hunks)
• inst/stan/data/simulation_rt.stan (1 hunks)
• inst/stan/estimate_infections.stan (5 hunks)
• inst/stan/estimate_secondary.stan (2 hunks)
• inst/stan/estimate_truncation.stan (4 hunks)
• inst/stan/simulate_infections.stan (2 hunks)
• inst/stan/simulate_secondary.stan (2 hunks)
• man/cash-.estimate_secondary.Rd (1 hunks)
• man/estimate_infections.Rd (1 hunks)
• man/estimate_secondary.Rd (1 hunks)
• man/extract_delays.Rd (1 hunks)
• man/extract_latent_state.Rd (1 hunks)
• man/extract_parameter.Rd (0 hunks)
• man/extract_parameter_samples.Rd (1 hunks)
• man/extract_parameters.Rd (1 hunks)
• man/extract_stan_param.Rd (1 hunks)
• man/get_predictions.Rd (1 hunks)
• man/get_samples.Rd (1 hunks)
• man/plot.forecast_secondary.Rd (1 hunks)
• man/summary.estimate_secondary.Rd (1 hunks)
• tests/testthat/test-checks.R (5 hunks)
• tests/testthat/test-delays.R (1 hunks)
• tests/testthat/test-estimate_infections.R (1 hunks)
• tests/testthat/test-estimate_secondary.R (8 hunks)

💤 Files with no reviewable changes (1)

• man/extract_parameter.Rd

🧰 Additional context used

🪛 LanguageTool

CLAUDE.md

[style] ~39-~39: Would you like to use the Oxford spelling “Standardize”? The spelling ‘Standardise’ is also correct.
Context: ...does, never reference issues - Good: "Standardise return structure for estimate_secondary...

(OXFORD_SPELLING_Z_NOT_S)

---

[style] ~40-~40: Would you like to use the Oxford spelling “Standardize”? The spelling ‘Standardise’ is also correct.
Context: ...cture for estimate_secondary" - Bad: "Standardise return structure for estimate_secondary...

(OXFORD_SPELLING_Z_NOT_S)

---

[uncategorized] ~48-~48: The preposition ‘to’ seems more likely in this position.
Context: ...ents - Follow the pull request template in .github/PULL_REQUEST_TEMPLATE.md - Co...

(AI_HYDRA_LEO_REPLACE_IN_TO)

NEWS.md

[style] ~21-~21: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...intervals merged with observations. - Use summary(object) to get summarised par...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)

---

[style] ~21-~21: Would you like to use the Oxford spelling “summarized”? The spelling ‘summarised’ is also correct.
Context: ...tions. - Use summary(object) to get summarised parameter estimates. Use `type = "compa...

(OXFORD_SPELLING_Z_NOT_S)

---

[style] ~21-~21: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... to get summarised parameter estimates. Use type = "compact" for key parameters o...

(ENGLISH_WORD_REPEAT_BEGINNING_RULE)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)

• GitHub Check: build (ubuntu-latest, release, https://packagemanager.rstudio.com/cran/__linux__/noble/latest)
• GitHub Check: ubuntu-latest (release)
• GitHub Check: ubuntu-latest (oldrel-1)
• GitHub Check: ubuntu-latest (devel)
• GitHub Check: windows-latest (release)
• GitHub Check: macOS-latest (release)
• GitHub Check: test-coverage
• GitHub Check: stan-model-benchmark
• GitHub Check: R-CMD-as-cran-check
• GitHub Check: synthetic-validation
• GitHub Check: lint-changed-files
• GitHub Check: pkgdown

🔇 Additional comments (58)

man/extract_latent_state.Rd (1)

5-5: Verify that the title case change aligns with project documentation conventions.

The title has been changed from title case ("Extract Samples for a Latent State from a Stan model") to sentence case ("Extract samples for a latent state from a Stan model"). Whilst this is a minor cosmetic change with no functional impact, please confirm that this capitalization shift is:

1. Part of an intentional documentation style standardization across the Rd files
2. Aligned with the project's Rd documentation conventions

If this change is incidental and not part of a broader documentation style update, consider reverting it to maintain consistency with established Rd documentation practices.

R/utilities.R (1)

509-510: LGTM!

The addition of "parameter" to globalVariables() is appropriate and follows standard R package conventions for preventing CMD check warnings about non-standard evaluation usage, particularly in data.table operations within the new parameter extraction infrastructure.

man/extract_parameter_samples.Rd (1)

5-5: LGTM! Documentation style updated.

The title casing has been correctly updated to sentence case, aligning with the project's style guidelines.

man/extract_stan_param.Rd (1)

5-5: LGTM! Documentation style updated.

The title casing has been correctly updated to sentence case, consistent with the project's style guidelines.

CLAUDE.md (1)

29-51: LGTM! Helpful workflow documentation added.

The new sections provide clear guidance on the PR workflow, commit message conventions, and NEWS.md practices. The content aligns well with the project's documented preferences.

R/stanmodels.R (1)

17-30: LGTM! Formatting improvements.

The reformatting improves readability without any functional changes.

R/extract.R (2)

60-103: LGTM! Well-structured multi-parameter extraction.

The function correctly builds a reverse lookup from param_id_* variables and handles both named and unnamed parameters with appropriate fallbacks.

---

116-171: LGTM! Robust delay parameter extraction.

The function correctly uses the delay ID lookup system with delay_types_groups to extract and name delay parameters. The boundary checks are appropriate for a groups array where id + 1 is accessed.

tests/testthat/test-estimate_infections.R (1)

33-38: LGTM! Good test coverage for the new accessor.

The test correctly validates that get_predictions() returns predictions with the expected structure and required columns.

R/simulate_secondary.R (1)

73-74: LGTM! Parameter naming standardised.

The parameter rename from delay to reporting aligns with the broader naming standardisation across the codebase.

R/estimate_truncation.R (1)

187-187: LGTM! Parameter naming standardised.

The parameter rename from trunc to truncation aligns with the broader naming standardisation across the codebase, improving consistency.

inst/stan/data/rt.stan (1)

8-8: Rename to delay_id_generation_time is consistent with new delay interface

The rename to delay_id_generation_time with int<lower = 0> cleanly reflects the generation-time delay identifier and matches the R-side create_stan_delays() output. No issues from a Stan data or indexing perspective.

inst/stan/data/simulation_observation_model.stan (1)

6-6: Truncation delay ID rename aligns with new delay semantics

delay_id_truncation with lower bound 0 is a clear, semantically accurate replacement for the previous truncation ID, and fits the new scheme alongside delay_id_generation_time and delay_id_reporting.

R/estimate_infections.R (2)

76-78: Return-value documentation matches the implemented interface

The updated description of the <estimate_infections> object (fit, args, observations) and the recommendation to use summary(), get_samples(), and get_predictions() now matches the actual return structure at the bottom of the function and the accessor-based workflow introduced elsewhere in the package.

---

268-271: Renamed delay arguments correctly wired into create_stan_delays()

Passing generation_time = generation_time, reporting = delays, and truncation = truncation into create_stan_delays() is consistent with the new delay_id_generation_time, delay_id_reporting, and delay_id_truncation fields produced on the R side and consumed by the Stan data blocks. The time_points expression is unchanged and remains appropriate for weighting delay priors.

inst/stan/data/simulation_delays.stan (1)

28-28: delay_id_reporting improves clarity for reporting delays

Renaming the generic delay ID to delay_id_reporting makes it explicit that this identifier refers to the reporting-delay component, which is helpful now that generation-time and truncation delays have their own IDs. The type and bounds remain appropriate.

man/estimate_infections.Rd (1)

107-111: Updated Rd value section is consistent with the function’s API

The Rd now correctly describes the <estimate_infections> object fields and points users towards summary(), get_samples(), and get_predictions(), matching the R documentation and implementation.

inst/stan/data/simulation_rt.stan (1)

8-8: Simulation RT data now uses delay_id_generation_time consistently

Using delay_id_generation_time here keeps the simulation RT data block aligned with the main RT data (inst/stan/data/rt.stan) and with the R-side create_stan_delays() output. No issues spotted.

R/simulate_infections.R (1)

137-141: LGTM! Semantic parameter names improve clarity.

The updated parameter names (generation_time, reporting, truncation) are more descriptive than the previous abbreviated versions and align with the broader refactoring across the codebase.

inst/stan/estimate_truncation.stan (2)

17-17: LGTM! Semantic delay ID naming enhances clarity.

The introduction of delay_id_truncation as a named constant improves code readability and aligns with the semantic delay ID system implemented across the codebase.

---

29-29: Consistent use of delay_id_truncation throughout the model.

All references to the truncation delay ID have been updated consistently, including array dimensions, PMF retrieval, and loop bounds.

Also applies to: 41-49, 82-84, 93-93

man/extract_delays.Rd (1)

1-22: LGTM! Clear documentation for internal extraction function.

The documentation accurately describes the extract_delays() function's purpose, usage, and return value. Properly marked as internal with the \keyword{internal} tag.

R/checks.R (1)

202-219: LGTM! Truncation checks updated consistently.

The update from trunc_id to delay_id_truncation is applied consistently across null checks and index calculations, preserving the original logic whilst aligning with the new semantic naming convention.

tests/testthat/test-checks.R (2)

210-210: LGTM! Test data updated for new delay identifier.

The test data correctly uses delay_id_truncation instead of trunc_id, maintaining test coverage for the renamed identifier.

Also applies to: 286-286

---

231-243: LGTM! Test calls updated with semantic parameter names.

The create_stan_delays() calls now use the new semantic parameter names (generation_time, reporting, truncation), ensuring tests validate the updated API.

Also applies to: 264-268

man/get_predictions.Rd (1)

1-45: LGTM! Comprehensive documentation for new accessor method.

The documentation clearly describes the get_predictions() S3 generic and its methods across multiple model classes. The return value, usage, and examples are well documented.

inst/stan/simulate_secondary.stan (2)

56-67: LGTM! Reporting delay logic updated with semantic identifiers.

The update from delay_id to delay_id_reporting and the use of reporting_rev_pmf as the variable name improve code clarity. The convolution logic remains correct.

---

81-92: LGTM! Truncation logic updated with semantic identifiers.

The update from trunc_id to delay_id_truncation and the use of trunc_rev_cmf as the variable name maintain consistency with the broader refactoring whilst preserving the truncation logic.

man/estimate_secondary.Rd (1)

94-98: LGTM! Documentation updated for new S3 interface.

The return value documentation now accurately reflects the standardised object structure (fit, args, observations) and the new accessor methods (summary(), get_samples(), get_predictions()), aligning with the PR objectives.

inst/stan/data/observation_model.stan (1)

8-9: LGTM! Semantic delay ID naming improves clarity.

The renaming from trunc_id to delay_id_truncation and delay_id to delay_id_reporting aligns well with the semantic naming convention introduced across the codebase. This enhances readability and makes the purpose of each identifier explicit.

tests/testthat/test-delays.R (6)

97-113: LGTM! Comprehensive test coverage for delay ID creation.

The test correctly verifies that create_stan_delays produces the expected delay_id_* variables with sequential IDs when all delay types are provided. The assertions properly check both the presence of the variables and their values.

---

115-127: LGTM! Correctly tests secondary model delay ID assignment.

The test appropriately verifies that when only a subset of delays is provided (reporting and truncation for secondary models), the IDs are still assigned sequentially starting from 1.

---

129-139: LGTM! Properly validates behaviour for missing delays.

The test correctly verifies that when only generation_time is provided, the corresponding delay_id_generation_time is created whilst delay_id_reporting and delay_id_truncation are absent from the data structure.

---

141-160: LGTM! Validates extract_delays behaviour with new naming system.

The test effectively verifies that extract_delays correctly processes the new delay_id_* variables and produces properly structured output with semantic parameter names (e.g., generation_time[1], generation_time[2]).

---

162-166: LGTM! Appropriate edge case handling.

The test correctly verifies that extract_delays returns NULL when delay_params are absent, ensuring graceful handling of incomplete data structures.

---

168-179: LGTM! Ensures backward compatibility with fallback naming.

The test appropriately validates that extract_delays gracefully handles cases where delay_id_* variables are absent by falling back to indexed naming (delay_params[...]), maintaining backward compatibility.

man/plot.forecast_secondary.Rd (1)

1-28: LGTM! Clear documentation for the new plotting method.

The documentation for plot.forecast_secondary is well-structured and provides appropriate guidance for users. The parameter descriptions are clear, and the lifecycle badge appropriately marks this as stable functionality.

man/get_samples.Rd (1)

7-8: LGTM! Consistent extension of accessor methods.

The addition of get_samples methods for estimate_secondary and forecast_secondary properly aligns with the existing pattern for infection models, providing a consistent interface across the package.

Also applies to: 17-19

inst/dev/stan-to-R.R (3)

120-125: LGTM! Consistent delay ID rename for generation time.

The update to use delay_id_generation_time (previously gt_id) maintains the existing logic whilst adopting the new semantic naming convention.

---

149-155: LGTM! Reporting delay ID consistently updated.

The rename from delay_id to delay_id_reporting correctly applies the semantic naming pattern to the reporting delay path in convolve_to_report.

---

166-172: LGTM! Truncation delay ID properly renamed.

The conditional check and subsequent get_delay_rev_pmf call correctly use delay_id_truncation (previously trunc_id), maintaining consistency with the new naming scheme.

man/cash-.estimate_secondary.Rd (1)

1-27: LGTM! Clear deprecation guidance for backward compatibility.

The documentation effectively communicates the deprecation of the old structure and provides explicit guidance for migrating to the new accessor methods (get_predictions(), get_samples(), and direct $observations access).

R/summarise.R (1)

889-945: LGTM! Well-structured summary method for estimate_secondary.

The implementation properly:

• Retrieves samples via get_samples(object)
• Filters to non-time-varying parameters using is.na(date)
• Applies appropriate summary statistics with configurable credible intervals
• Supports both compact and filtered parameter views

The regex filtering on line 938 correctly matches key parameter patterns, providing sensible defaults for the compact view.

man/summary.estimate_secondary.Rd (1)

1-40: LGTM! Comprehensive documentation for summary method.

The documentation clearly describes the summary.estimate_secondary method's behaviour, including the distinction between "compact" and "parameters" types, and provides appropriate guidance for the return value structure.

R/format.R (1)

194-209: Bulk parameter extraction and naming look consistent with new infrastructure

Using extract_parameters(samples) once and then looping over unique(all_params$parameter) is a sensible consolidation. The switch() mapping of "dispersion" → "reporting_overdispersion" and "frac_obs" → "fraction_observed" should preserve backwards‑compatible semantics while standardising names, and the nrow(result) > 0 guard avoids inserting empty components.

I would just suggest double‑checking that downstream consumers expect these new list element names (rather than the raw Stan parameter names) everywhere this helper is used, to avoid subtle regressions.

inst/stan/estimate_secondary.stan (1)

59-68: Reporting delay refactor matches the new delay ID scheme

The new delay_id_reporting path, with vector[delay_type_max[delay_id_reporting] + 1] reporting_rev_pmf and a matching get_delay_rev_pmf(..., delay_type_max[delay_id_reporting] + 1, ...) feeding convolve_to_report(scaled, reporting_rev_pmf, 0), is consistent with the updated delay‑ID design and mirrors the patterns in the infections models.

This looks correct and should help avoid the “wrong delay id in observation model” bug you’re fixing.

NAMESPACE (1)

4-22: S3 registrations and exports are aligned with the new interface

The added S3method() entries for get_predictions, get_samples, summary.estimate_secondary, plot.forecast_secondary, and the $ method for estimate_secondary, together with export(get_predictions), look consistent with the PR’s S3 design (mirroring estimate_infections / forecast_infections).

Assuming the corresponding generics and methods exist in the roxygen‑annotated R sources, this NAMESPACE wiring should behave as intended and survive re‑generation.

Also applies to: 34-35, 42-43, 91-93

inst/stan/simulate_infections.stan (1)

60-128: Delay ID refactor in simulation model is coherent and dimensionally consistent

The simulation model’s switch to delay_id_generation_time, delay_id_reporting, and delay_id_truncation looks internally consistent:

• gt_rev_pmf, reporting_rev_pmf, and trunc_rev_cmf are all declared with delay_type_max[delay_id_*] + 1 and passed the same size into get_delay_rev_pmf, avoiding dimension mismatches.
• The order of operations (use gt_rev_pmf in generate_infections, then convolve with reporting_rev_pmf, optionally truncate with trunc_rev_cmf, then apply scale_obs if obs_scale) is clear and mirrors the estimation model.

I don’t see any obvious correctness issues in this block.

inst/stan/estimate_infections.stan (1)

78-107: Generation‑time, reporting, and truncation delays now consistently use semantic delay IDs

The changes here look internally coherent:

• gt_rev_pmf is now keyed off delay_id_generation_time and sized as estimate_r * (delay_type_max[delay_id_generation_time] + 1), with the corresponding get_delay_rev_pmf() call using delay_type_max[delay_id_generation_time] + 1 as its max_k argument.
• Reporting and truncation both use their respective IDs (delay_id_reporting, delay_id_truncation) with delay_type_max[delay_id_*] + 1 in both the declaration and the get_delay_rev_pmf() call, then feed the resulting kernels into convolve_to_report() and truncate_obs() exactly as in the simulation model.
• In the generated‑quantities block, gt_rev_pmf_for_growth and sampled_gt_rev_pmf also consistently use delay_type_max[delay_id_generation_time] + 1, which should keep calculate_Rt() and calculate_growth() aligned with the updated generation‑time specification.

Given the subtle size arithmetic, it would be worth double‑checking that get_delay_rev_pmf()’s return shape for the generation‑time ID still matches the estimate_r * (...) layout expected by generate_infections(), but structurally these changes look sound.

Also applies to: 139-152, 175-188, 268-283

R/get.R (1)

245-261: get_samples.estimate_infections implementation looks consistent with existing patterns

The method cleanly reuses extract_samples() plus format_samples_with_dates() and back-fills missing args entries; this is in line with the new standardised interface and should give a coherent date/variable/sample/value layout without altering existing semantics. I don’t see issues here.

tests/testthat/test-estimate_secondary.R (3)

38-92: Tests correctly exercise new accessors and S3 shape for estimate_secondary

Switching to

inc_posterior <- get_samples(inc)[variable %in% params]
prev_posterior <- get_samples(prev)[variable %in% params]
predictions <- get_predictions(inc)
posterior <- get_samples(inc)

and asserting

expect_equal(names(inc), c("fit", "args", "observations"))
expect_s3_class(inc, "estimate_secondary")
expect_s3_class(inc, "epinowfit")
expect_equal(
  names(predictions),
  c(
    "date", "primary", "secondary", "accumulate", "median", "mean", "sd",
    "lower_90", "lower_50", "lower_20", "upper_20", "upper_50", "upper_90"
  )
)

is a good, direct check that the new API (fit/args/observations + accessors) behaves as intended.

No issues from a testing perspective here.

---

156-182: Parameter recovery tests are well-adapted to get_samples

The updated recovery tests now compute summaries directly from get_samples():

inc_summary <- inc_posterior[, .(
  mean = mean(value),
  median = stats::median(value)
), by = variable]

(and similarly for prev and the cmdstanr backend), then compare these to the known simulated parameters.

This approach cleanly validates the new extraction pipeline (via extract_parameters() under the hood) under both backends and doesn’t hard-code any assumptions about the internal list structure of the fit. Looks solid.

Also applies to: 194-208

---

258-294: Nice alignment of deprecated preprocessing tests with the new return structure

The tests for filter_leading_zeros and zero_threshold now only check that:

• The output object remains of class "estimate_secondary", and
• It exposes the standardised structure:

expect_named(out, c("fit", "args", "observations"))
expect_equal(get_predictions(out)$primary, modified_data$primary)

This keeps validation focused on observable behaviour while respecting the new fit/args/observations pattern. No issues here.

R/estimate_secondary.R (5)

59-63: Return-value documentation matches the new S3 structure

The updated @return description:

• Describes an <estimate_secondary> object with fit, args, and observations, and
• Points users to summary(), get_samples(), and get_predictions(),

is consistent with the actual construction of ret later in the function and with the expectations in the tests. This is clear and accurate.

---

235-238: Named arguments to create_stan_delays align with the new delay ID semantics

Switching the delay setup to:

stan_data <- c(stan_data, create_stan_delays(
  reporting = delays,
  truncation = truncation,
  time_points = stan_data$t
))

brings estimate_secondary() in line with the semantic reporting/truncation interface used elsewhere and should help avoid the kind of delay-ID mismatch that motivated this PR. This looks correct and keeps the Stan data more self-documenting.

---

269-277: Standardised epinowfit/estimate_secondary return structure looks good

The new return block:

ret <- list(
  fit = fit,
  args = stan_data,
  observations = reports
)
class(ret) <- c("epinowfit", "estimate_secondary", class(ret))

achieves the PR goal of aligning estimate_secondary() with estimate_infections():

• fit carries the Stan fit for diagnostics,
• args exposes the full Stan data list, and
• observations contains the cleaned report data.

Tests confirm the expected names(inc) and S3 classes, so this structure is coherent and forward-compatible with the new accessors.

---

373-421: plot.estimate_secondary correctly switches to accessor-based predictions

Having plot.estimate_secondary() start with:

predictions <- get_predictions(x)

instead of relying on a $predictions field ensures that:

• The plot always uses the canonical prediction computation, and
• The same code can be reused for forecast_secondary objects via their own get_predictions() method.

The rest of the function remains unchanged and still respects primary, from/to, and optional new_obs. This refactor is clean and keeps plotting aligned with the new accessor-based API.

---

424-439: plot.forecast_secondary reuse is simple and effective

The new method:

plot.forecast_secondary <- function(x, primary = FALSE,
                                    from = NULL, to = NULL,
                                    new_obs = NULL,
                                    ...) {
  plot.estimate_secondary(
    x, primary = primary, from = from, to = to, new_obs = new_obs, ...
  )
}

nicely reuses the existing plotting logic while dispatching through the correct get_predictions() method for forecast_secondary objects. This keeps the visual behaviour consistent between fitted and forecast objects without duplicating code.

[github-actions]

Thank you for your contribution sbfnk 🚀! Your synthetic_recovery markdown is ready for download 👉 here 👈!
_{(The artifact expires on 2025-12-02T14:07:32Z. You can re-generate it by re-running the workflow here.)}

[github-actions]

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 8069e53 is merged into main:
Further explanation regarding interpretation and methodology can be found in the documentation.

[sbfnk]

stan model benchmark failure is expected - this is still struggling with changes in variable names in the stan code.

[github-actions]

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 3293941 is merged into main:
Further explanation regarding interpretation and methodology can be found in the documentation.