Remove jurisdiction as a threaded dimension#51
Merged
Conversation
Drop the jurisdiction dimension from the timeseries wrapper layer: as_matrix() now returns a plain date-indexed vector instead of a date x jurisdiction matrix, and requires target_infection_dates so every vector is reindexed onto a shared master date axis at construction time rather than having its date range inferred from the data (fill_date_gaps() is removed). The create_epiwave_*_timeseries() constructors drop their jurisdictions parameter accordingly. Part of the jurisdiction-dimension removal refactor (see plan at .claude/plans/logical-waddling-reef.md).
Drop the n_juris_ID indexing parameter now that obs_data/obs_prop are target_infection_dates-aligned vectors rather than jurisdiction-columns of a matrix. This also fixes a pre-existing bug: obs_prop[n_juris_ID] used linear indexing into a date x jurisdiction matrix and silently returned a single scalar (row n_juris_ID) instead of that jurisdiction's proportion series; obs_prop is now indexed consistently with obs_data by date. Also explicitly drops observable dates that fall outside target_infection_dates (e.g. a delay-shifted date walking off the front edge of the window), which previously could produce an NA in a subscripted assignment downstream.
…tion Drop target_jurisdictions and the associated column-matching/reordering logic entirely. Move DOW correction out of this function (it now needs n_jurisdictions, which this layer no longer knows about; DOW gets applied later by stack_jurisdictions()). Build the forward convolution matrix here, directly from this jurisdiction's own delay distribution, replacing the lapply-over-jurisdictions convolution-matrix construction that used to live in create_observation_model()/create_small_sero_model(). Collapse the per-jurisdiction inits loop and matrix-reassembly into a single direct inits_by_jurisdiction() call. Also fixes a field-name bug: seroprevalence data was stored as size_vec but create_small_sero_model() read size_mat (always NULL); size_vec is now properly coerced via as_matrix() and carried through to be renamed/stacked correctly at the jurisdiction-combining step. Drops the vestigial inform_inits field, which was never set by define_observation_data()/ define_sero_data() and never consumed downstream. Manually verified against a synthetic single-jurisdiction stream: all returned vectors are target_infection_dates-length and correctly aligned.
…ction
Drop target_jurisdictions and the dead x parameter (never had a default, no
call site ever supplied it, and the function body never referenced it).
Replace the pmax()-over-2D-arrays / abind-into-3D-array + apply(mean) logic
with plain 1-D Reduce('|', ...) and rowMeans(cbind(...)) now that each
stream's prepare_observation_data() output is a vector rather than a
jurisdiction-matrix. Drop the now-unused abind import.
define_observation_data()/define_sero_data() keep their existing argument
names/count unchanged; only their docs are updated to note that
total_pop/size_vec/proportion_infections are now scoped to a single
jurisdiction per call, since these are called once per jurisdiction going
forward.
Manually verified: a two-stream (cases + hospitalisations) bundle for one
jurisdiction produces correctly shaped, correctly aligned output.
This is the one place jurisdiction becomes a dimension again. It takes a named list of per-jurisdiction observation model bundles (each already 1-D, from define_observation_model()) and stacks their vectors into the date x jurisdiction matrices that create_infection_timeseries(), create_observation_model()/create_small_sero_model(), and create_dow_priors() already expect -- those functions' existing n_jurisdictions-parameterised, shared-hyperparameter behaviour (partial pooling) is unchanged, just fed from a different source. A length-1 list collapses to today's single-jurisdiction case. DOW correction moves here from prepare_observation_data(), since applying it requires knowing n_jurisdictions, which per-jurisdiction prep structurally doesn't. Requires every jurisdiction to supply the same set of streams and identical dow_model flags per stream, erroring otherwise -- both are intentional, documented limitations for now rather than permanent constraints. Also stacks total_pop/size_vec into total_pop/size_mat for seroprevalence streams. Because every per-jurisdiction vector coming out of layer 1/2 is already target_infection_dates-length and positionally aligned (the master-date-axis invariant enforced by as_matrix()), stacking here is a plain cbind() with no date-reconciliation logic needed. Manually verified with two jurisdictions on staggered, non-identical date ranges, a seroprevalence stream, and dow_model = TRUE on the cases stream: shapes are correct, the alignment check confirms row i means the same calendar date in every jurisdiction's column, the hierarchical DOW branch (create_dow_priors(2)) runs without error, and the mismatched-dow_model / unnamed-list error paths fire with clear messages.
create_small_sero_model() Both functions now read convolution_matrices, case_mat, prop_mat (and, for sero, size_mat/total_pop) directly from stack_jurisdictions()'s output instead of rebuilding a per-jurisdiction convolution matrix list themselves via lapply(unique(delays$jurisdiction), ...) -- that logic now lives once, in prepare_observation_data(), rather than being duplicated across these two functions. Also deletes the now-dead data_idx/expected_cases_idx trimming step (it inferred which rows of a full-length convolution output to keep by matching case_mat's rownames-derived date range; case_mat is now guaranteed target_infection_dates-length and positionally aligned by construction, so expected_cases is already the right shape). This incidentally fixes create_small_sero_model(), which was unreachable in practice: its convolution-matrix block called new_convolution_matrix(delays, x, n_dates), a 3-argument call that doesn't match the current 2-argument new_convolution_matrix(pmf, n) signature, and it read a size_mat field that prepare_observation_data() never actually set (it stored size_vec instead -- fixed in a prior commit). Both are resolved automatically now that convolution matrices and size_mat arrive pre-built and correctly named. Dropped the now-unused infection_days parameter from both functions (it was only used by the deleted convolution-matrix-building and data_idx logic). Manually verified: building the greta graph for both a cases stream and a sero stream from stacked two-jurisdiction data produces correctly-named, correctly-shaped (150 x 2) outputs with no errors.
…gthscale Rename the observations parameter to observations_by_jurisdiction and make the first line of the function stack_jurisdictions(observations_by_jurisdiction) -- this is what lets the common single-jurisdiction case stay a one-call ergonomic path (a length-1 named list) while keeping the stacking logic factored out into its own, independently-testable function. Fix the sero/count dispatch: previously create_small_sero_model()'s call was commented out, so every stream -- including seroprevalence -- was unconditionally run through create_observation_model()'s negative-binomial cases likelihood. Streams are now dispatched by the presence of total_pop. Fix greta::initials(gp_lengthscale = rep(0.5, n_jurisdictions)): gp_lengthscale is a scalar node in create_infection_timeseries() regardless of n_jurisdictions (it's a shared kernel hyperparameter across jurisdiction columns), so rep(..., n_jurisdictions) would produce a dimension mismatch on the first real n>1 fit; this was previously never exercised since the package has never been run with more than one jurisdiction. Manually verified end-to-end: a single-jurisdiction synthetic fit (flat_prior, small MCMC settings) runs to completion with correctly-shaped output.
Run devtools::document() after the R/ changes: exports stack_jurisdictions, drops importFrom(abind, abind)/importFrom(methods, is)/importFrom(rlang, .data)/importFrom(tidyr, pivot_wider) (all now unused after the jurisdiction- dimension refactor), and adds importFrom(greta, binomial) (now used directly in create_small_sero_model()'s roxygen tags). Drop abind/methods/rlang from DESCRIPTION Imports accordingly (confirmed via grep no remaining usage of any of the three anywhere in R/). Drop "jurisdiction" from R/epiwave-package.R's globalVariables() -- its only NSE use site (dplyr::filter(jurisdiction == ...) in the old inits_by_jurisdiction()) was removed in an earlier commit. Verified via devtools::check() that this doesn't reintroduce a NOTE (the remaining `~jurisdiction` reference in plot_infection_traj.R's facet_wrap() is a formula, not flagged by codetools the way NSE dplyr columns are). devtools::check() otherwise shows 0 errors; the remaining WARNINGs/NOTEs (undeclared cli/distributional imports, hidden .github, draw/value bindings in plot_infection_traj.R) all trace to files this refactor doesn't touch and are pre-existing on the simplify branch.
These aren't automated tests (no tests/testthat/ suite exists), but they're the primary way this pipeline gets manually exercised against real data, so keep them runnable: drop jurisdictions=/target_jurisdictions= args from create_epiwave_greta_timeseries()/create_epiwave_massfun_timeseries()/ define_observation_model() calls, and wrap each single-jurisdiction define_observation_model() bundle in a named list (setNames(list(...), jurisdictions)) before passing it to fit_waves(observations_by_jurisdiction = ...). testing.R's sero total_pop = c(8e6, 7e6) becomes a scalar (total_pop = 8e6), reflecting that define_sero_data() is now called once per jurisdiction. Also fixes a pre-existing typo in testing.R (fit_waves(..., infection_model = 'gp_growth_rate')) to the actual parameter name, infection_model_type -- unrelated to this refactor, but the line was already being touched.
… wrapper epiwave.params now has native discrete_pmf_series/discrete_weights_series objects (new_discrete_series(), with their own date-based subsetting, validation, print/summary methods). epiwave's own epiwave_massfun_timeseries wrapper predates these and is now purely redundant: prepare_observation_data() was coercing delay_from_infection into that wrapper tibble and then immediately reconstructing a discrete_pmf_series from it via new_discrete_series() -- a pointless round-trip. Remove create_epiwave_massfun_timeseries()/epiwave_massfun_timeseries entirely. prepare_observation_data() now accepts delay_from_infection as either a single discrete_pmf/discrete_weights object (replicated across target_infection_dates via new_discrete_series()) or an already time-varying discrete_pmf_series/discrete_weights_series (aligned via the series' own Date-based subsetting), and builds the convolution matrix directly from that series object. Widening to accept discrete_weights (not just discrete_pmf) matters for the seroprevalence pathway specifically: unlike case/hospitalisation notification (a one-time event, correctly modelled as a normalised discrete_pmf), seroconversion is typically persistent -- a person may test positive for many consecutive days -- so it's better represented as an unnormalised discrete_weights curve.
delays is now a discrete_pmf_series/discrete_weights_series object (see previous commit), not a data.frame/tibble, so dplyr::filter(delays, date %in% case_dates) no longer works -- replaced with the series' own Date-based subsetting, delays[case_dates]. Also fixes a live bug: expected_delay_vals was computed as sum(x$delay * x$mass), but discrete_pmf objects have columns step/prob, not delay/mass -- those fields don't exist, so this silently evaluated to 0 every time (the mean delay used to shift observed dates into inferred infection dates was always treated as zero, regardless of the actual delay distribution). Now uses epiwave.params's mean.discrete_pmf(), generalised to discrete_weights via epiwave.params::normalise() first (weights aren't a proper distribution, so they're normalised to a pmf before taking a mean). Verified directly: for a gamma(shape=3, rate=0.5) delay (true mean 6, vs 0 before this fix), expected_delay_vals now correctly computes ~6.
Mechanically identical to the discrete_pmf path (a day-difference matrix looked up against the object's step column), just using $weight instead of $prob. This is what makes the seroprevalence pathway able to use an unnormalised persistence curve (discrete_weights/discrete_weights_series) instead of being forced into a normalised discrete_pmf. Verified standalone: new_convolution_matrix() on a discrete_weights object produces row sums that don't sum to 1 (confirming weights aren't being force-normalised), and a discrete_weights_series built from a single replicated discrete_weights object produces an identical matrix to the single-object path.
…adoption
Document the discrete_pmf vs discrete_weights choice in
create_small_sero_model()/define_observation_data()/define_sero_data()'s
roxygen: sero streams should typically supply delay_from_infection as a
discrete_weights/discrete_weights_series persistence curve, not a normalised
discrete_pmf.
Fix tests/test_workflow/{testing.R,test2.R}: epiwave.params::add_distributions()
no longer exists (renamed to add_discrete()/the + operator) -- these calls
were already stale before this branch, unrelated to the jurisdiction
refactor, but directly relevant now that this pass touches the discrete
object plumbing throughout. Also drop testing.R's now-unnecessary
create_epiwave_massfun_timeseries() wrapping step, since
prepare_observation_data() accepts a raw discrete_pmf directly.
devtools::document() after the discrete-series adoption changes: drops the create_epiwave_massfun_timeseries export/man page, updates man pages for the touched functions. Also declares cli (Imports) and distributional (Suggests) in DESCRIPTION -- new_convolution_matrix.R calls cli::cli_abort()/cli::cli_warn() directly (including two new call sites added in this pass) and its @examples block uses distributional::dist_gamma(), neither of which were formally declared despite being used directly (previously only working because epiwave.params happens to depend on both transitively). devtools::check() now shows 0 errors, 0 warnings (down from 2 warnings); the remaining 3 NOTEs are pre-existing and trace to files this refactor doesn't touch.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Part 1: remove jurisdiction as a threaded dimension.
jurisdictionwas previously threaded through nearly every function as an explicit second dimension: matrices becamedate x jurisdiction, convolution operators becamelapply-built lists of matrices, inits were assembled viaabindinto 3D arrays, and greta arrays neededsweep/apply(MARGIN=2)calls to broadcast per jurisdiction-column. This restructures the pipeline into three layers so jurisdiction is a dimension only in the last one, while preserving (not losing) hierarchical/partially-pooled multi-jurisdiction fitting:prepare_observation_data()) — now purely 1-D vectors in, 1-D vectors out.define_observation_model()) — bundles cases/hospitalisations/sero for one jurisdiction, still 1-D. User-facingdefine_observation_data()/define_sero_data()signatures are unchanged.stack_jurisdictions(), called internally byfit_waves(), takes a named list of per-jurisdiction bundles and stacks them into the matricescreate_infection_timeseries()/create_observation_model()/create_dow_priors()already expect. Those three functions keep their existing shared-hyperparameter, partial-pooling behaviour essentially unchanged — a length-1 list collapses to today's single-jurisdiction numbers.A master-date-axis invariant is enforced explicitly: every per-jurisdiction vector coming out of layer 1 is guaranteed
length(target_infection_dates)and positionally aligned to it at construction time (as_matrix()now requirestarget_infection_datesand reindexes onto it, rather than inferring a date range from the data, which only worked before because all jurisdictions were pivoted together in one call). This is what makes stacking safe across jurisdictions with different/staggered data coverage.Part 2: adopt epiwave.params's native discrete series objects.
epiwave.paramsnow has nativediscrete_pmf_series/discrete_weights_seriesobjects (new_discrete_series(), with their own date-based subsetting/validation). epiwave's ownepiwave_massfun_timeserieswrapper predated these and was purely redundant —prepare_observation_data()was coercing delays into that wrapper and immediately reconstructing adiscrete_pmf_seriesfrom it. That wrapper is removed;prepare_observation_data()now consumesdiscrete_pmf/discrete_weights(or their already-time-varying_seriesforms) directly.new_convolution_matrix()/evaluate()are widened to acceptdiscrete_weightsalongsidediscrete_pmf, since seroconversion (unlike a one-time notification event) is typically persistent and better modelled as an unnormalised weights curve than a normalised PMF.Bugs fixed along the way (all in files this refactor already rewrites):
inits_by_jurisdiction()'sobs_prop[n_juris_ID]used linear indexing into a matrix, silently returning one scalar instead of a jurisdiction's proportion series.create_small_sero_model()'s call infit_waves()was commented out, it callednew_convolution_matrix()with a stale 3-argument signature, and it read asize_matfield that was never actually set (size_vecwas stored instead). All three are fixed as a natural consequence of every stream (cases, hospitalisations, sero) flowing through the same unified pipeline.greta::initials(gp_lengthscale = rep(0.5, n_jurisdictions))would have produced a dimension mismatch on the first real n>1 fit, sincegp_lengthscaleis a scalar shared kernel hyperparameter regardless ofn_jurisdictions.inits_by_jurisdiction()'sexpected_delay_valswas computed assum(x$delay * x$mass), butdiscrete_pmfobjects have columnsstep/prob, notdelay/mass— this silently evaluated to 0 always (mean delay treated as zero). Now usesepiwave.params::mean.discrete_pmf()(generalised todiscrete_weightsvianormalise()first).tests/test_workflow/{testing.R,test2.R}calledepiwave.params::add_distributions(), which no longer exists in the currentepiwave.params(renamed toadd_discrete()/+).Also declares
cli(Imports) anddistributional(Suggests) inDESCRIPTION— both were used directly innew_convolution_matrix.Rwithout being formally declared.Full design writeup and rationale for part 1: see the plan this branch implements (
.claude/plans/logical-waddling-reef.mdin the local Claude Code plan directory, not part of the repo).Test plan
No automated test suite exists yet (
tests/testthat/isn't populated). Verified viadevtools::load_all()+ synthetic reprexes at each stage:devtools::check()— 0 errors, 0 warnings (remaining 3 NOTEs are pre-existing, in files this branch doesn't touch)flat_prior, small MCMC settings): correct output shapes, finite/non-negative posterior drawsdow_model = TRUE: correct shapes, hierarchical GP and DOW pooling both execute, and the critical alignment check confirms row i means the same calendar date in every jurisdiction's column (verified at a date only jurisdiction A has data, and a date only jurisdiction B has data)discrete_pmf, a pre-builtdiscrete_pmf_series, and adiscrete_weightspersistence curve for sero) runs end-to-end; confirmed the sero convolution matrix is genuinely unnormalised (row sums ≠ 1) andmean.discrete_pmf()-based inits now compute the correct non-zero delay shift (previously silently 0)tests/test_workflow/testing.R/test2.Rupdated to the new call pattern and parse cleanly (can't run to completion — depend on local, not-synced data)🤖 Generated with Claude Code