Skip to content

Forward n_timepoints and first_day_dow to ascertainment models - #891

Closed
developer-rpai wants to merge 1 commit into
CDCgov:mainfrom
developer-rpai:890-time-varying-ascertainment
Closed

developer-rpai wants to merge 1 commit into
CDCgov:mainfrom
developer-rpai:890-time-varying-ascertainment

Conversation

@developer-rpai

Copy link
Copy Markdown

Closes #890.

What the issue is

MultiSignalModel computes the shared model-axis length and first_day_dow and forwards both to the latent process and every observation process — but calls each registered AscertainmentModel.sample() with no arguments. That prevents ascertainment models with temporal processes (e.g. time-varying ascertainment) from producing full-axis, calendar-aligned trajectories, even though the AscertainmentModel.sample(**kwargs) contract explicitly allows model-context arguments.

Approach

  • MultiSignalModel.sample() now passes n_timepoints (shared axis length, n_init + n_days_post_init) and first_day_dow to each ascertainment model's sample(). Existing scalar implementations (JointAscertainment, RatioLinkedAscertainment) already accept **kwargs and ignore them, so this is backward-compatible.
  • Added AscertainmentModel.requires_calendar_anchor() (default False, mirroring the latent-process API). Subclasses sampling a calendar-aligned temporal process can override it to return True.
  • _check_obs_start_date() now includes ascertainment models when deciding whether obs_start_date is required, so a missing calendar anchor fails fast at the model entry with a clear message naming the offending ascertainment model. This covers both sample() and validate_data() (both call _check_obs_start_date).
  • Documented the two context kwargs on the base AscertainmentModel.sample() docstring and on MultiSignalModel's ascertainment_models parameter.

Tests

New tests in test/test_pyrenew_builder.py (TestTimeVaryingAscertainment, 4 tests) and test/test_ascertainment.py (TestRequiresCalendarAnchor, 2 tests), using a time-varying ascertainment test double with a day-of-week effect that requires both kwargs and requires_calendar_anchor() == True:

  • the model forwards the correct n_timepoints and first_day_dow, and the sampled trajectory is calendar-aligned to the supplied obs_start_date;
  • missing obs_start_date raises from both sample() and validate_data() naming the ascertainment model;
  • negative controls: JointAscertainment output is byte-identical with and without obs_start_date (new kwargs safely ignored); and the 3 new behavior tests fail against the unfixed code while the scalar test passes either way;
  • requires_calendar_anchor() defaults to False for both scalar implementations.

Local results: 105 passed across test/test_pyrenew_builder.py + test/test_ascertainment.py, 6 passed in the two weekly-ascertainment integration tests; ruff check and ruff format --check clean. I ran these with jax 0.11.2 / numpyro 0.22.0 in a local venv — upstream CI is authoritative for the full matrix.

…v#890)

MultiSignalModel now passes n_timepoints (shared model-axis length) and
first_day_dow (axis-origin day-of-week) when sampling registered
ascertainment models, so ascertainment models with temporal processes
can produce full-axis, calendar-aligned trajectories. Existing scalar
ascertainment implementations accept **kwargs and are unaffected.

Adds AscertainmentModel.requires_calendar_anchor() (default False,
mirroring the latent-process API) and includes ascertainment models in
MultiSignalModel's obs_start_date entry check.

Tests: time-varying day-of-week ascertainment test double verifies the
forwarded axis context and calendar alignment; anchor checks fire from
both sample() and validate_data(); scalar JointAscertainment output is
unchanged with and without the new kwargs; requires_calendar_anchor()
defaults to False for scalar implementations.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟢 Approval recommended

The implementation fulfills the issue requirements with appropriate validation, documentation, and regression coverage.

Review effort: Balanced
Findings: None

What changed in this PR

Forwards model-axis timing context to ascertainment models, enabling calendar-aligned, time-varying ascertainment.

Changes:

  • Passes n_timepoints and first_day_dow during ascertainment sampling.
  • Adds calendar-anchor requirements and validation.
  • Adds coverage for temporal and scalar ascertainment behavior.
File Description
pyrenew/​ascertainment/​base.py Defines and documents the calendar-anchor contract.
pyrenew/​model/​multisignal_model.py Forwards timing context and validates required anchors.
test/​test_ascertainment.py Tests default anchor behavior.
test/​test_pyrenew_builder.py Tests forwarding, alignment, validation, and compatibility.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@cdc-mitzimorris

Copy link
Copy Markdown
Collaborator

thank you for this PR, but this issue is not ready for implementation. see #890 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support time-varying ascertainment on the shared model axis

3 participants