fix(da): five data assimilation correctness fixes - #125
Merged
Conversation
A directory written at another cadence substitutes every file, on every window, for every arm. Naming each distinct requested and used pair keeps a directory that changes product partway from reporting only one of them.
A pruned branch with no flow-frame column left the confluence denominator short, so a surviving sibling took the share the stopped branch owns. The in-kernel wrapper settles every refusal before subtracting the gage nudge, which otherwise republished the gage with its own nudge removed.
An empty frame cannot say whether its DA type is switched on, so load_state both erased parameters a run had just built and let a run with the type off launder stale ones into the next. create_state now records which types produced the frames. An empty lastobs frame drops streamflow DA to open loop, so it takes the same guard.
Each block gates on the observation frame and then reads the parameter frame, so a mismatch died on a column lookup naming nothing useful. Raises only when the job has waterbodies of that type: with none selected the lookups are .loc[[]], which the empty frame serves.
load_state rewound the clock before the restore guards ran, so a refused load left the model half applied and an identical retry accepted it. The saved-off exemption also kept frames postdating the restored time, which the kernel reads update_time from. lastobs launders like the reservoir frames, so it now reads the nudging flag.
The partition only reaches the result once the DA has a time span. At zero span the spread is output-only and applied per timestep, and splitting a window is bit-identical, so refusing a short update there made the shipped Standard AnA config unrunnable: 3 forcing columns against a max_loop_size default of 24.
Omitting a block leaves it present and null after validation, while consumers reach through it with chained .get, so a config that never mentioned reservoir_da killed the NHF build on NoneType.get. Doing this inside model_dump covers every caller; four entry points build this dict and two of them had no wrapping.
The kernel re-seeds its at-gage lastobs per window and update_after_compute persists it only for nudging, so a gage with an observation gap is nudged differently across a boundary even at zero span. Only an update capped to its own forcing is one window and therefore safe; a memory-driven split stays fatal.
The kernel re-seeded lastobs from each window's own t0 observation, so a gage that fell silent stopped being corrected at whatever boundary max_loop_size landed on and a chunked AnA cycle differed from an unchunked one. The scaling arm drives the same nudging override, so the harvest reads what the kernel already records. The frame rides in the BMI checkpoint, so the continuity spans cycles too.
The merge globbed the output directory and replaced files[0] with everything it found, so an AnA cycle sharing a directory destroyed the previous cycle's published results. Also retires the window guard's stated reason, which cited a per-window lastobs reset that no longer happens.
The reader interpolates across gaps in both directions, so the filled value at a timestamp depends on how much series was loaded. Giving each window only its own TimeSlice files therefore made the injected observations, and the discharge, depend on max_loop_size: measured on VPU 01 at 52.1 cms in the injected frame and 76.6 cms in routed flow, against 0.0 once every window reads the same run-spanning list. Separate BMI update calls still differ, which is real-time semantics rather than a defect. Nudging and reservoir persistence keep their per-window lists.
A scaling run discarded its own checkpointed lastobs, because the restore asked whether legacy nudging was on rather than whether this run owns the frame. It now asks the latter, and trims the restored rows to the roster this run assimilates: a nudging checkpoint carries holdout, reservoir-routed and co-located gages that the scaling arm excludes, and installing them raised "not in index" downstream. The lake output merge also globbed the whole directory, so a later AnA cycle destroyed the previous cycle's published file the way the stream merge already did.
Measurements belong in the PR, not in every docstring that cites them.
Both arms default off, and the scaling path never reads streamflow_nudging, so removing nudging later stays seamless. This is the seam that would rot silently.
0 is the new default and means automatic: 24 forcing columns, floored at the DA span, capped by memory. Explicit values are honored unchanged. Both drivers log the window they picked. The estimate behind that cap read 21x under the Tier A sweep's measured peak, and cpu_pool was applied as a multiplier where measured tree PSS at pool 8 is 1.13x the main process. Both are calibrated now, with one test anchored on the published numbers instead of on the helper it tests. Also folds a final window shorter than the DA span into the one before it, matching the CLI. Automatic sizing produces such remainders routinely.
24 was being demanded of any update, so an 18 column update against a 12 hour span failed a run that is correct as one window. Only the span is a floor; the window is a preference. An explicit max_loop_size still refuses to shrink, since that value is a promise about the DA's operating window. The refusal no longer tells an operator to lower a knob they never set.
A window longer than the update cannot bound memory, since the update already caps it, so refusing over one stopped a run for nothing. The hard error stays where no window choice helps: an update shorter than the DA span.
Folding a short final window into its neighbor ran after the memory cap and could push that window past it: 47 columns at a cap of 23 came out as one window of 47.
They chunked their own way, so 100 columns at a width of 24 gave [24, 24, 24, 28] on one driver and [20] x 5 on the other. Both now call troute.window_plan.
psutil reads the host, which is not what a container may use. Also says once, on the second update, that update cadence is result-bearing under a DA span.
Read as one per-element constant, Ohio gives 409 B and CONUS 31 B. The per-column term is 94% of a small domain's slope, so fitting Ohio alone put CONUS at 152 GB a window and refused any scaling run there. Numbers in benchmark/RESULTS.md 5b.
The DA adds 67.8 MB per column and 13 B per element, where it was modeled as +34 B per element and nothing per column. Cluster runs assimilate, so this is the arm that matters.
ceil(nts / ceil(required / available)) rounds the wrong way and gave a 4-column window against a budget of 3.4, 18% over. Only a non-integral budget exposes it. An exhausted cgroup also divided by zero.
Collapsing to one window turned a 2161-column run at span 60 into a single 2161-column window, and the CLI has no memory cap to catch it. Also: the DA figures carry no Courant block, since the sweep ran with the lag off.
A Slurm or PBS job cgroup is nested below the mount root, so reading the root found no limit and returned host RAM. Walks this process's own cgroup and takes the smallest limit up the tree, or Slurm's stated allocation.
Cut to what would stop the next reader undoing the change; numbers and provenance stay in benchmark/RESULTS.md 5b. Also two British spellings.
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
1 participant
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.
Data assimilation correctness fixes, found while chasing a
KeyError: 'totalCounts'whena short-range RFC forecast restored a warm-up state written with assimilation off. Each
was a run that died with an error naming the wrong thing, or exited 0 while assimilating
less than it reported.
Additions
span_da_runs, one TimeSlice list covering every window. The reader's gap fill isnon-local, so a per-window list made the injected observations depend on
max_loop_size.Model._reservoir_da_enabledandModel._streamflow_nudging_enabled, plus areservoir_da_enabledentry in the serialized state, so a restore can tell adeliberately empty frame from a starved one.
_require_reservoir_da_paramsincompute.py, naming an observation/parametermismatch instead of dying on a column lookup.
max_loop_size.0is the new default and means 24 forcing columns,floored at the DA span and capped by memory. Both drivers log the window they picked.
normalization, and the run window guard.
Removals
GageTree.with_positions, and the module logger ingage_tree.pythat only served it.Changes
streamflow_nudgingdefaults toFalse. The scaling DA replaces it. Nudging staysavailable for comparison.
with_positionsraises on an unresolvable pruned branch and the caller drops the site,rather than returning a partial tree whose confluence denominator is short.
apply_in_kernelsettles every refusal before subtracting a gage's nudge, whichotherwise stayed subtracted with nothing to restore it.
load_staterestores DA frames by the run's config rather than by emptiness, resolvesevery frame before mutating anything, and keeps live state only until the model routes.
lastobsis harvested for either streamflow arm, so stale-obs decay survives a windowboundary.
benchmark/RESULTS.md. It read 21x under measured peak, andcpu_poolwas applied asa multiplier where the measured pool cost is 1.13x.
requested -> usedpair, not one example.Testing
pixi run pytest -m "not integration": 503 passed, 5 skipped, 1 xfailed.0.00000000on both drivers. Before the observationfix the same comparison gave 76.55 cms over 73,553 cells.
withdrawal.
guard restored.
ruffclean on all new modules by path. No new rule types on touched files.run to confirm it assimilates rather than merely not crashing.
Screenshots
N/A, no user facing surface.
Todos
Checklist
Testing checklist
Target Environment support
Accessibility
Other