Skip to content

Mem bugfix 903 subpop scaling - #904

Merged
dylanhmorris merged 27 commits into
mainfrom
mem_bugfix_903_subpop_scaling
Oct 14, 2026
Merged

dylanhmorris merged 27 commits into
mainfrom
mem_bugfix_903_subpop_scaling

Conversation

@cdc-mitzimorris

@cdc-mitzimorris cdc-mitzimorris commented Oct 12, 2026 •

Copy link
Copy Markdown
Collaborator

Closes #903

Problem

The latent process returns infections per resident of each subpopulation. MultiSignalModel.sample() turned these into counts by multiplying by population_size alone, so each subpopulation was scaled as if it held the whole jurisdiction. As a result, "subpop" observations such as SubpopulationCounts received counts too large by a factor of 1 / subpop_fractions[k]. The subpopulation counts did not sum to the aggregate count, and fitted Rt was biased by subpopulation size.

Fix

In MultiSignalModel.sample():

  • "subpop" observations receive all_subpops * population_size * subpop_fractions[k]. latent_infections_by_subpop records the same values.
  • A new resolution, "subpop_per_capita", passes all_subpops through unchanged. MeasurementObservation.infection_resolution() now returns it. A wastewater concentration does not depend on how many people live in the catchment, so measurement observations need infections per resident. Under the corrected "subpop", their predictions would have scaled with subpop_fractions[k].

Breaking change: MeasurementObservation subclasses now receive infections per resident instead of infections per resident times population_size. Scale parameters such as genomes shed per infection must be set for the new input.

Tutorials

  • building_multisignal_models.qmd: the Infection Resolution section now describes all three resolutions and how latent infections are converted for each. log10_genome_per_inf changes from 9.0 to 4.1, so predicted concentrations match the scale of fake_nwss.csv.
  • observation_processes_measurements.qmd: the example infections are now per resident.
  • Small wording and typo fixes in ascertainment.qmd, latent_subpopulation_infections.qmd and observation_processes_counts.qmd.

Tests

On the pre-fix code, 7 of the 9 new tests fail. All 9 pass with the fix.

  • test/test_multisignal_infection_scaling.py: traces the model and checks what each resolution receives. Subpopulation counts equal infections per resident times subpopulation population and sum to the aggregate. Measurement observations receive infections per resident. PopulationInfections is unchanged.
  • test/integration/test_subpopulation_counts_scaling.py: fits SubpopulationInfections to counts simulated for four subpopulations with fractions 0.5, 0.25, 0.15 and 0.10, all with the same true Rt. Before the fix, Rt in the smallest subpopulation was about 14% low over the first three weeks, and the subpopulation Rt deviations trended with subpopulation size (slope 0.032; the test requires below 0.01).
  • test/conftest.py now forces 64-bit floats and fails any test that runs without them. StubMeasurementObservation moves to test/test_helpers.py.

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.

🔵 Needs a closer look

The prerequisite #897 remains unmerged, and the stacked changes include unresolved numerical and documentation issues.

5 open findings
What changed in this PR

Fixes #903 by scaling subpopulation infections to their population sizes and routing per-capita infections to measurement observations. It also includes the still-unmerged #897 ascertainment work.

Changes:

  • Corrects aggregate, subpopulation-count, and per-capita infection routing.
  • Adds scalar and time-varying ascertainment after delay convolution.
  • Expands unit, integration, and tutorial coverage.
File Description
pyrenew/​model/​multisignal_model.py Corrects infection scaling and routing.
pyrenew/​model/​pyrenew_builder.py Documents ascertainment registration.
pyrenew/​observation/​base.py Adds delay-only convolution helper.
pyrenew/​observation/​count_observations.py Applies ascertainment after delay.
pyrenew/​observation/​measurement_observations.py Requests per-capita infections.
pyrenew/​observation/​__init__.py Documents infection resolutions.
pyrenew/​convolve.py Adds delay-only convolution API.
pyrenew/​ascertainment/​base.py Adds temporal ascertainment support.
pyrenew/​ascertainment/​independent.py Adds independent ascertainment model.
pyrenew/​ascertainment/​joint.py Supports temporal joint rates.
pyrenew/​ascertainment/​linked.py Supports temporal ratio-linked rates.
pyrenew/​ascertainment/​__init__.py Exports the new model.
test/​test_multisignal_infection_scaling.py Tests corrected infection routing.
test/​test_observation_counts.py Tests time-varying ascertainment.
test/​test_observation_validation.py Reuses shared test helper.
test/​test_interface_coverage.py Updates measurement resolution expectation.
test/​test_helpers.py Centralizes measurement test double.
test/​test_convolve.py Tests delay-only convolution.
test/​test_ascertainment.py Tests expanded ascertainment behavior.
test/​test_pyrenew_builder.py Tests builder integration.
test/​conftest.py Enforces 64-bit JAX tests.
test/​integration/​conftest.py Adds integrated ascertainment fixtures.
test/​integration/​test_subpopulation_counts_scaling.py Tests fitted subpopulation scaling.
test/​integration/​test_population_infections_e_rtdaily_rw_timevary.py Tests time-varying ED ascertainment.
test/​integration/​test_population_infections_he_weekly_joint_ascertainment.py Updates joint-site assertions.
test/​integration/​test_population_infections_he_weekly_ratio_linked_ascertainment.py Updates ratio-site assertions.
docs/​tutorials/​ascertainment.qmd Documents ascertainment models.
docs/​tutorials/​building_multisignal_models.qmd Documents infection routing.
docs/​tutorials/​observation_processes_counts.qmd Documents post-delay ascertainment.
docs/​tutorials/​observation_processes_measurements.qmd Uses per-capita infections.
docs/​tutorials/​latent_infections.qmd Applies formatting cleanup.
panache.toml Removes Python lint configuration.

🧠 Review effort: Balanced


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

Comment thread pyrenew/ascertainment/base.py
Comment thread test/test_helpers.py Outdated
Comment thread docs/tutorials/ascertainment.qmd Outdated
Comment thread docs/tutorials/ascertainment.qmd Outdated
Comment thread pyrenew/ascertainment/base.py
@github-actions

github-actions Bot commented Oct 12, 2026 •

Copy link
Copy Markdown

Thank you for your contribution @cdc-mitzimorris 🚀! Your github-pages is ready for download 👉 here 👈!
(The artifact expires on 2026-10-21T16:21:18Z. You can re-generate it by re-running the workflow here.)

@codecov

codecov Bot commented Oct 12, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.84%. Comparing base (cffa5dd) to head (aa0773e).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #904   +/-   ##
=======================================
  Coverage   98.83%   98.84%           
=======================================
  Files          59       59           
  Lines        2241     2242    +1     
=======================================
+ Hits         2215     2216    +1     
  Misses         26       26           
Flag Coverage Δ
unittests 98.84% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@cdc-mitzimorris
cdc-mitzimorris changed the base branch from main to mem_890_time_varying_ascertainment October 13, 2026 18:31
Base automatically changed from mem_890_time_varying_ascertainment to main October 13, 2026 19:42
@cdc-mitzimorris
cdc-mitzimorris force-pushed the mem_bugfix_903_subpop_scaling branch from 6e213d1 to 39508f0 Compare October 13, 2026 20:26

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.

🟢 Approval recommended

The implementation matches the required scaling semantics, with only a minor documentation grammar correction remaining.

1 open finding
5 resolved since last review

🧠 Review effort: Balanced

Comment thread docs/tutorials/ascertainment.qmd Outdated
Comment thread docs/tutorials/building_multisignal_models.qmd Outdated
Comment thread docs/tutorials/building_multisignal_models.qmd Outdated

@dylanhmorris dylanhmorris left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Throughout: replace "resident of subpopulation" with "member of subpopulation" since subpopulations are not necessarily spatial locations.

cdc-mitzimorris and others added 4 commits October 13, 2026 18:08
Co-authored-by: Dylan H. Morris <dylanhmorris@users.noreply.github.com>
Co-authored-by: Dylan H. Morris <dylanhmorris@users.noreply.github.com>
@cdc-mitzimorris

Copy link
Copy Markdown
Collaborator Author

@dylanhmorris requested changes made.

Comment thread pyrenew/model/multisignal_model.py Outdated
Comment thread test/integration/test_subpopulation_counts_scaling.py Outdated
Comment thread test/integration/test_subpopulation_counts_scaling.py Outdated
Comment thread test/integration/test_subpopulation_counts_scaling.py Outdated
Comment thread test/integration/test_subpopulation_counts_scaling.py
Comment thread test/conftest.py Outdated
@dylanhmorris

Copy link
Copy Markdown
Collaborator

Assert that MeasurementObservation.infection_resolution() is subpop_per_capita" here?

# infection_resolution() coverage

@dylanhmorris dylanhmorris left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks @cdc-mitzimorris. Spotted a few small things reviewing the tests, but once those are addressed I think this is good to go.

cdc-mitzimorris and others added 17 commits October 14, 2026 11:25
Co-authored-by: Dylan H. Morris <dylanhmorris@users.noreply.github.com>
Co-authored-by: Dylan H. Morris <dylanhmorris@users.noreply.github.com>
Co-authored-by: Dylan H. Morris <dylanhmorris@users.noreply.github.com>
Co-authored-by: Dylan H. Morris <dylanhmorris@users.noreply.github.com>
Co-authored-by: Dylan H. Morris <dylanhmorris@users.noreply.github.com>
Co-authored-by: Dylan H. Morris <dylanhmorris@users.noreply.github.com>
Co-authored-by: Dylan H. Morris <dylanhmorris@users.noreply.github.com>
Co-authored-by: Dylan H. Morris <dylanhmorris@users.noreply.github.com>
Co-authored-by: Dylan H. Morris <dylanhmorris@users.noreply.github.com>
Co-authored-by: Dylan H. Morris <dylanhmorris@users.noreply.github.com>
Co-authored-by: Dylan H. Morris <dylanhmorris@users.noreply.github.com>
Co-authored-by: Dylan H. Morris <dylanhmorris@users.noreply.github.com>
Co-authored-by: Dylan H. Morris <dylanhmorris@users.noreply.github.com>
@cdc-mitzimorris

Copy link
Copy Markdown
Collaborator Author

Assert that MeasurementObservation.infection_resolution() is subpop_per_capita" here?

# infection_resolution() coverage

Already covered in test_measurements_infection_resolution (test/test_interface_coverage.py:197): it asserts "subpop_per_capita" using a concrete subclass that inherits MeasurementObservation.infection_resolution() unchanged.

@cdc-mitzimorris

Copy link
Copy Markdown
Collaborator Author

@dylanhmorris - all changes made - thanks!

@dylanhmorris dylanhmorris left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@dylanhmorris
dylanhmorris merged commit 0bd0853 into main Oct 14, 2026
8 checks passed
@dylanhmorris
dylanhmorris deleted the mem_bugfix_903_subpop_scaling branch October 14, 2026 20:10
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.

Subpopulation infection counts are scaled by the total population instead of the subpopulation population

3 participants