Add dynamic adjustment to realistic, global ocean workflow - #644
Draft
xylar wants to merge 270 commits into
Draft
Conversation
Collaborator
Author
|
I have only tests: with MPAS-Ocean and Gnu compilers on Chrysalis. it passes, but I haven't looked at output carefully. I am able to set up: and the processor count now makes sense (it was far too large before recent debugging). I'm launching a job now but likely won't have time to look at the results for a few days at least. |
xylar
force-pushed
the
add-realistic-global-ocean-dynamic-adjustment
branch
5 times, most recently
from
August 6, 2026 11:55
61a02c8 to
60d461d
Compare
xylar
added
enhancement
land_frac and ocean_frac are fractions in [0, 1] that claimed to be a bedrock elevation in metres: the masks they come from are built by comparing base_elevation against zero, and xarray carries its attributes through the comparison. Label the derived masks in MaskTopoStep instead of letting them inherit. grounded_frac and ice_frac, which had no long_name or units at all, pick theirs up from the combined topography now that it is labelled. Also drop cell_measures, coordinates and grid_mapping from the remapped fields. ncremap stamps the first two on everything it writes and this step drops area, lat and lon; grid_mapping arrives from the projected Antarctic source and names a variable that does not survive remapping either. All three were left pointing at variables that are not in the file. Copy attributes as a dict rather than by reference, so a masked field and its source no longer share one attrs dict. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
out_filename was computed once, outside the loop over components, from whichever component the preceding loop happened to leave bound -- the last step's. Caching steps from two components in one invocation wrote both sets of entries to a single file named after one of them, and the load at the top of the loop read the wrong component's json into cached_files. Move the assignment inside the loop. Also write the trailing newline json.dump leaves off, so a generated cached_files.json no longer needs one added by hand. Found while looking at what re-running polaris cache on the same day does; it has nothing to do with attributes, but it is a small fix in a file this branch already touches. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Labelling the combined fields wholesale dropped every inherited attribute, including the one that was right: BedMachine and Bedmap3 both supply standard_name = land_ice_thickness, which really does describe what the field holds. Put it back. The other five fields get no standard_name. bedrock_altitude and sea_area_fraction look like candidates, but neither has been checked against the CF standard-name table, and an unverified standard name is worse than none. This lands after the topography was regenerated, so the cached files do not carry it; it will appear the next time the topography is rebuilt. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The p-star initialization path bypassed init_vertical_coord/add_1d_grid, so pstar_init.nc lacked the 1D reference vertical coordinate variables (refBottomDepth, refZMid, refTopDepth, refInterfaces). MPAS-Ocean requires refBottomDepth as an input on its mesh stream. Reuse the existing add_1d_grid() in the base PStarInitStep output assembly so every p-star init step produces these fields, and expose a shared REF_COORD_VARS constant for downstream routing/exclusion. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The base p-star init now emits refBottomDepth/refZMid/refTopDepth/ refInterfaces for MPAS-Ocean. Omega uses RefPseudoThickness instead and ignores these, so drop them from the Omega initial state file to keep its output minimal and model-appropriate. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Assert the base PStarInitStep output contains refBottomDepth and its companions, verify they are kept for MPAS-Ocean but dropped for Omega in write_initial_state_dataset, and guard that REF_COORD_VARS stays in sync with add_1d_grid. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
pyremap's Remapper defaults to use_tmp=True, which puts the SCRIP files and the MOAB .h5m files in a TemporaryDirectory() under /tmp. On Chrysalis (and many other HPC machines) /tmp is a node-local disk, so the MPI tasks running mbtempest on other nodes cannot see the files and abort during the parallel read. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The final savefig used bbox_inches='tight' on a fixed-aspect cartopy GeoAxes, which shrinks the map axes so only part of the domain is drawn -- Antarctica was cut off the bottom of every global lat-lon plot. This is the same failure mode fixed for plot_global_mpas_field in "Fix half-globe clipping in plot_global_mpas_field"; that fix was never propagated to plot_global_lat_lon_field. The fix there was to let constrained_layout manage the margins, but that does not work here: this function attaches its colorbar with inset_axes anchored outside the axes, which constrained_layout does not account for, so the colorbar falls off the canvas entirely. Reserve the margins explicitly instead, which keeps the full domain, the colorbar and its tick labels. Affects all callers of plot_global_lat_lon_field: the woa23 and jra55 hydrography/forcing viz, the e3sm/init combined-topography viz and the unified base-mesh viz. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
cartopy's Gridliner draws its own tick labels and constrained_layout does not measure them, so the labels that stick out furthest were clipped at the figure edge. On Robinson the 30-degree labels sit left of the 60-degree ones, so "30N"/"30S" rendered as bare "N"/"S" while "60N"/"60S" survived. Reserve the horizontal margin explicitly on the layout engine. Drawing the figure once before saving, so the labels exist for a second layout pass, does not work -- constrained_layout still does not account for them. Companion to "Fix domain clipping in plot_global_lat_lon_field"; affects every caller of plot_global_mpas_field. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A get_*_steps() helper is called once per consumer, and get_or_create_shared_step already makes that safe for the steps. The config those steps use had no equivalent, so a helper that built it unconditionally handed the second caller a *different* config object at the same filepath while the shared steps -- created on the first call -- went on using the first one. Both failure modes are bad. Quietly, options set on what the second caller was handed reach nothing. Loudly, passing it to Task.set_shared_config raises, because add_config refuses a different config at a path that already has one. So Component gains get_or_create_shared_config, the companion to get_or_create_shared_step, taking a setup callback that runs only when the config is really created. It registers the config immediately, which is what lets the second caller find it. _get_target_topo_steps was building its config unconditionally and now goes through it. Eight other modules already hand-roll the same guard, which is why the gap was easy to miss; converting those is left for later, since they are correct as they stand. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three tests on the helper itself: that it creates and registers, that it returns the same object on a second call, and that the setup callback runs only once -- otherwise a second caller would re-add its packages on top of options the first caller's steps had already been given. Three more that the real helpers hand back the config their steps are using, for woa23, jra55 and the lat-lon combined topography. Each was checked against the unfixed code: reverting a helper fails its test on the identity assertion, so the tests cover the bug rather than just the new method. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The shared-steps section of the developer guide gains the companion method and, more usefully, why building a shared config unconditionally is a bug even though it looks harmless: the second caller gets a different object at the same path while the steps keep the first, so its options reach nothing and registering it raises. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Step.add_dependency was the odd one out among Polaris' registration methods. Component.add_step and Component.add_config both raise only when a *different* object is registered at the same key and treat re-registering the same object as a no-op; add_dependency raised on any repeat of the name. That punishes exactly the pattern shared steps are built around. A get_*_steps() helper is called once per consumer, so a dependency it wires outside a step constructor -- a restart chain, whose links only the helper knows -- was wired again on the second call and raised. The workaround is for the helper to track whether each call is the one that created the step, which is bookkeeping the framework should not be asking for. So the same step under the same name now returns early. Early, not just past the raise: the rest of the method calls add_output_file() and add_input_file(), neither of which de-duplicates. The error worth keeping is the name collision between two different steps, which is what the `name` argument exists to resolve, and that one still raises -- with a message that now says which case it is. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three tests: that add_dependency wires the pickle output and input that carry a dependency, that repeating it changes nothing -- checked all the way down to the output and input lists, since those are what would grow if the method fell through rather than returning -- and that a different step under a name already in use still raises. The second and third fail against the previous behaviour, so they cover the change rather than just the method. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Wiring inside the step constructor is the pattern to reach for, since get_or_create_shared_step() passes constructor arguments only when it really creates the step. Where the helper is the only thing that knows the link, add_dependency() may now simply be called again. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Seven helpers still hand-rolled the guard this method replaces:
if filepath in component.configs:
return component.configs[filepath]
They were correct, which is why the helper that was missing it went
unnoticed until it handed a second caller a config its own shared steps
were not using. Having one way to do this is what stops that recurring,
so the four unified-mesh helpers, topo/cull, topo/remap and mesh/base/add
now go through the method too.
That needed the method to change shape. Three of the seven do not build
a config themselves; they delegate to a builder that creates and returns
one -- get_unified_mesh_config and get_sizing_field_config. A callback
handed a config to populate cannot express those without duplicating
what the builders do or changing their signatures, so the callback now
*returns* the config instead. An existing builder can be passed
straight through, and the method's job reads as what it is: caching, not
construction.
Because a builder could return a config with any filepath, one that does
not match the key is now an error -- it would be registered under a path
it does not know about, which is the same silent mismatch this method
exists to prevent.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The "sets up only once" test becomes "builds only once" against the new callback, and gains an assertion that the second call returns the same object rather than only the same contents. A new test covers the filepath check: a builder that ignores the filepath raises, and nothing is registered under the key. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The example builds and returns a config rather than filling one in, and the text notes that an existing builder can be passed straight through, along with the filepath requirement that makes that safe. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A get_*_steps() helper suggests one symlink name per step, chosen for
consumers whose tasks live elsewhere in the tree, and tasks forward those
names verbatim:
for symlink, step in steps.items():
self.add_step(step, symlink=symlink)
Four tasks -- CullTopoTask, RemapTopoTask, Woa23 and Jra55 -- live in the
directory that holds those very steps, so the symlink lands next to what
it points to: cull_mask beside mask, woa23_viz beside viz. 105 of the
3074 step symlinks in the tree are like that. They are noise, and a
second name for a directory already in view is worse than no name.
add_step now drops those rather than asking every such task to filter
the names itself, which is the same mistake waiting to be made a fifth
time. Paths are compared in full, since two components can have the
same subdirectory layout and a step from another component is somewhere
else on disk however its subdirectory reads.
A symlink to a step nested *deeper* under the task is kept -- the 40
like viz_remapped_unsmoothed_topo -> unsmoothed/viz. Surfacing a step
from further down under a descriptive name is what symlinks are for.
Symlinks are navigational only: step_symlinks is read in one place,
_setup_step, and the run machinery keys off step.name. Nothing about
what runs changes.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Five tests on add_step: a symlink to a step elsewhere is kept, one that would sit beside its step is dropped, one to a step nested deeper is kept, a step from another component with a coincidentally matching subdirectory keeps its symlink, and the degenerate case where the symlink would name the step itself is dropped. Two of the five fail against the previous behaviour. The two unified-topo tests asserted that every suggested name became a symlink. Their point -- that the task uses the factory's names rather than inventing its own -- is unchanged, so they now say that with the side-by-side ones excluded, and assert that the excluded set is not empty so the check cannot pass vacuously. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The shared-step section of the tasks guide now says that a symlink landing in the directory that already holds the step is dropped, why a task hits that at all -- it passes through names meant for consumers elsewhere -- and that a symlink to a step nested deeper is kept. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The u.oi6to18.lr6to10 first stage failed its temperature check on the WOA23 warm cell off Sumatra, which reaches 35.8 degC there rather than the 33.5 it reaches at 30 km because a 6-18 km mesh averages NOAA's bad source cell far less. It is also much slower to erode: 35.77 after an hour, 34.54 after two, and 34.29 at the end of the six-hour stage, which extrapolates to more than a day before it falls below 33. Skipping only the sample written before the first time step, as the checks did, leaves 35.77 as the first sample judged. So two changes, and both are needed. The thresholds move to 35 degC and 44 PSU, because a stage-1 exclusion alone cannot save a bound at 33: the artifact is still near 34.3 when stage 2 begins and near 33.9 when stage 3 does, so those stages would fail on it too. And the tracer extremes now skip a startup window as well as the first sample, because at 35 the first hour's 35.77 would still fail. The window is measured from the start of the sequence rather than from each stage, since what it skips belongs to the initial condition. A per-stage window would instead stop looking just after every damping change, which is where a schedule that steps the damping down too fast would show itself. For the same reason the CFL check does not get the window: the opening hours are where a time step that is too long shows up. With the default 2 hours, stage 1 is judged from hour 3 (34.53 degC, 42.60 PSU) and no later stage is affected at all. Lowering salinity_max from 45 rather than leaving it is a tightening the evidence now supports. A scan of the u.oi6to18.lr6to10 initial condition finds every cell above 42 PSU in the Red Sea artifact at 1150 m -- 26 of them, peaking at 44.13 -- and nothing hypersaline anywhere else, so 44 still clears the artifact while a real blow-up produces far more. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Ten tests, in three groups. The window arithmetic: it covers the first stage of the real u.oi6to18.lr6to10 schedule and no other, it carries what is left of itself into a second stage when the first is shorter than the window, and a zero-length window leaves only the first sample excluded. What the extremes then see: the real hourly series from the failing stage, which is the case worth pinning down, plus a window that lands exactly on a sample time (the model writes two hours as 0.0833..., so matching cannot rely on exact equality) and a stage that lies wholly inside the window and so has nothing to judge. What a stage check makes of it: the real series passes, a 99 degC sample after the window still fails, and a CFL spike in the first hour still fails -- the window is for the tracer artifacts, and a time step too long for the flow shows up exactly where the window sits. The summary test asserts the same window reaches the CSV, and only the tracer columns, so what a user reads cannot disagree with what passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The user guide gains the window itself, why it is measured from the sequence rather than from each stage, and why the CFL check does not get it. Its WOA23 paragraph now covers what the artifacts do at 6-18 km -- 35.78 degC rather than the 33.46 at 30 km, and slow enough to still be there when the second and third stages begin -- since that is what the thresholds are set against. Its config block had drifted from the file it quotes, missing salinity_max and cfl_max and still describing the kinetic-energy check that was replaced; it is now a copy of the real one. The developer guide describes first_judged_index as the one place that decides which samples a stage is judged on, and excluded_days_in_stage as what turns a sequence-wide duration into a per-stage one, including why the result is expressed relative to the stage. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Rebasing onto the fixed hmix scaling left two assertions behind: the u.oi6to18.lr6to10 config now asks for ref_cell_width against a 6 km reference rather than the meshDensity scaling that never worked. They also now check what the reference width and the del4 divergence factor are, since those are the part that actually differs per mesh -- E3SM's per-grid coefficients are referenced to the finest cells of their own mesh, so borrowing a coefficient without its reference width is how this goes wrong quietly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The stages, their checks, validate and viz were constructed directly by the task, so they lived only in that task. A downstream workflow that wants what the adjustment produces -- e3sm/init's component inputs want the relaxed restart -- could not reach them, and building its own copy would mean running the whole chain a second time. So they are now built the way every other unified-mesh workflow builds its steps: a get_realistic_dynamic_adjustment_steps() in steps.py that composes the shared init chain and creates each of its own steps through Component.get_or_create_shared_step, returning (steps, config, stages) and taking include_viz, exactly as get_realistic_init_steps does. The task keeps only what is particular to running the sequence standalone. The stages come back alongside the steps because which stage hands off the adjusted state, and what that restart file is called, both depend on the schedule: stages[-1].restart_out is what a consumer is after. Two things did not fit the pattern without care. The restart chain is wired with add_dependency rather than input/output files, because a stage's restart filename comes from the schedule rather than the step, and add_dependency raises on a duplicate -- so _get_or_create reports whether this call is what created the step, and only a new one is wired. And because the set of steps depends on the schedule, configure() still has to rebuild when a user's setup-time config changed it: a stage whose name survived the change would otherwise come back from the shared-step cache still carrying its old run duration. Only the adjustment steps are rebuilt, since re-requesting the init chain is not the same as leaving it alone -- it would build a second copy of the shared configs its own upstream steps own. StageCheck, Validate and VizDynamicAdjustmentStep take subdir rather than indir, which is what get_or_create_shared_step passes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Six tests for what "shared" has to mean here. That a second consumer gets the same instances rather than a second chain, and that asking twice does not re-wire the restart chain -- which would raise on the duplicate dependency. That every adjustment step is registered on the component under its work directory, which is what makes it reachable. That viz is created either way but only returned when asked for. That the last stage names the restart a consumer is after. And that the shared config is reused rather than rebuilt. The configure() test covers both halves of the rebuild: an unchanged schedule leaves every step alone, and a changed one replaces the stages -- including a stage whose name survived, which has to come back with the override's run duration rather than the built-in schedule's -- while leaving the init steps upstream untouched. The step helpers move from indir to subdir to match the constructors. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A "shared steps" section in the developer guide: what get_realistic_dynamic_adjustment_steps composes, why it hands back the stages as well as the steps and config, and the two details that do not follow the init pattern -- the restart chain wired with add_dependency, and the configure() rebuild that only touches the adjustment steps. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two workarounds went in with the shared steps, both for framework gaps that are now fixed on add-realistic-ocean-init and reached this branch with the rebase. _get_or_create tracked whether each call was the one that created a step, because Step.add_dependency raised on a repeat and the restart chain is wired from the helper rather than from a step constructor. add_dependency now returns early when the same step is added again, so the tracking and the two `if created:` guards are gone and the helper calls get_or_create_shared_step directly. get_adjustment_steps existed so that configure() could rebuild the stages without re-requesting the init chain, which used to build a second copy of the shared configs its upstream steps own. Component.get_or_create_shared_config fixed that, so there is one public builder again and no _init_steps held on the task. configure() still rebuilds only the steps under this mesh's dynamic_adjustment directory, but for a different reason than before, now stated: Task.remove_step carries through to the component once no task is using a step, so emptying the whole task would evict the init chain as well and leave a consumer holding stale instances. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The sharing test asserted only that a second call does not raise, which was what the removed `created` guard bought. Now that the chain is wired unconditionally, it snapshots every forward and check step's dependencies before and after the second call and requires them to match, with a guard that the snapshot is not vacuously empty. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The developer guide said the restart chain could only be wired for a newly created step and that configure() rebuilt through a separate builder to avoid re-requesting the init chain. Neither is true now: the chain is wired unconditionally because re-adding a dependency is a no-op, and configure() asks for the whole chain again. The reason the rebuild is still scoped to the adjustment steps is now the one that actually applies -- Task.remove_step reaches the component. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
check_ke_growth_decelerates and its two config options are removed, and Validate now only collects, writes and logs the diagnostics. The check was rewritten three times -- comparing kinetic-energy levels, then the fractional change stage over stage, then that change per unit time -- and every revision was forced by a healthy run failing it. The last was u.oi6to18.lr6to10, which reached the end of its schedule with a falling CFL, monotonically decaying tracer extremes and machine-noise tracer conservation, and was failed for a 178% change in mean kinetic energy across its final stage. The quantity is not comparable across stages, for three reasons at once. Stage lengths span 0.25 to 18 days, so a longer stage changes more for no interesting reason. The final stage switches the Rayleigh damping off, so its kinetic energy rises by design -- that is what the ramp is for. And a stage whose energy dips, as damped_adjustment_7 did, makes the next ordinary rise read as an acceleration, because the criterion takes a magnitude. Normalising by stage duration and restricting to the damped stages makes all three shipped meshes pass, but a 3% dip mid-ramp still fails it, so that was not a fix either. Three samples cannot separate any of that from a run that is genuinely running away. What remains are bounds on quantities with real failure modes behind them: the tracer maxima detect a blow-up, and the CFL number detects a time step too long for the flow. Whether an adjustment has settled is read from the diagnostics table and the viz figure, which show the whole series rather than three samples of it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The seven unit tests for check_ke_growth_decelerates and the three Validate tests that exercised it go with it. Two are replaced rather than deleted. A sequence whose mean kinetic energy accelerates -- the one the old check rejected -- now has to run cleanly and still write its summary, which pins the new behaviour rather than just removing the old. And the test that required at least ke_check_num_stages stages per unified schedule, whose only purpose was to keep that check running, is replaced by one asserting the damping ramps monotonically down, which is what the schedules are actually for. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both guides said validate applied a fourth, cross-stage check. They now say it applies none, and record why: the three rewrites, the three reasons the quantity is not comparable across stages, and that the judgement is made from the figure and the table instead. Written as a warning against re-adding it, since the reasoning is not obvious from the code that is left and this is the kind of metric that looks like an oversight. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
temperature_drift_per_day and salinity_drift_per_day are the change in a volume-weighted mean tracer across a stage, divided by the stage's run duration. "Drift" invited reading them as a settling diagnostic -- a mean still on the move meaning the stage had not finished adjusting -- and the comment above them said exactly that. They cannot mean that. These runs are forced by wind alone, with no surface fluxes, so the volume-weighted mean tracers are conserved exactly and the expected value of both columns is zero regardless of how settled the stage is. The completed runs bear that out: oi30 reaches -4e-17 degC/day and oi6to18 -6e-14, which is machine noise. A value larger than that says the model lost or gained tracer, which is worth knowing and is not what the name suggested. So they become mean_temperature_change_per_day and mean_salinity_change_per_day, describing the computation and leaving the interpretation to the comment, which now says the expected value is zero and why. The viz panels follow, since they plot the same quantity. Also drops a comment on the 'last' reduction that still referred to the settling check as its reason for existing; that check is gone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The two assertions that name the columns move to the new names, and the test that covers them is renamed to match: it records the mean tracer change over a stage, not a drift. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both guides described these columns as a settling signal -- the user's guide told a reader the "tracer drift should be shrinking" from stage to stage, which is not something a conserved quantity does. The user's guide now says what the columns are for, that both should be zero to within machine noise, and quotes what the completed oi30 and oi6to18 runs actually reached, so a reader has a number to compare against rather than a direction to look for. The developer's guide gains the same reasoning next to where the columns are derived. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
On u.oi6to18.lr6to10 the first five stages span 0.25 to 1 day on a 30-day axis, and their labels -- two lines of stage name over damping, lying flat -- overprinted into a smear that could not be read at all. The figure is what settling is judged from, so a label band that cannot be read is worse than a cosmetic problem. Three changes, all needed together. The labels rotate to read upwards, which cuts a label's footprint from its length to one line height. The names shorten from 'damped 1' to 'd1', and 'simulation' to 'sim', so a label is one short line rather than two. And because 0.25 days is 3 pt on that axis -- narrower than any legible label, rotated or not -- a label that still would not fit climbs to the next row instead of landing on its neighbour. Rows are assigned greedily, so the crowded opening stages climb and the wide ones that follow drop back to the bottom row. The stacking is measured in points from the top of the axes rather than in axes fractions, so it does not depend on how tall tight_layout ends up making the panel. The manual headroom this used to reserve is gone: tight_layout counts annotations in the axes' bounding box already, so reserving space as well pushed the panels an extra two inches down. Schedules whose stages are all comfortably wide -- every other mesh -- stay on one row and look as they did. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The short-name test follows the abbreviations, and gains a case for a name matching neither pattern, which is passed through. Two new tests cover the stacking, which is the part that could regress quietly: four 10-day stages must stay on one row, because stacking them would only waste headroom, and the u.oi6to18.lr6to10 opening must use more than one. The upper bound matters as much as the lower: eight rows would mean every stage had climbed rather than only the crowded ones. These assert on the row count rather than on measured text extents. A pixel-exact assertion would pin the result to a font, and the row count is what the layout decision actually is. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The users' guide said each stage is "labelled with its damping", which no longer tells a reader enough to find the label: it now reads upwards, the names are abbreviated, and on a crowded schedule it may sit a row or two above its neighbours. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Omega's GlobalStats moved from daily temporal reductions to snapshots on the configured cadence, which renamed the file it writes: the period is part of the name, so it is global_stats_1DayInstants for a daily stage and global_stats_1HourInstants for u.oi6to18.lr6to10's hourly ones. This module still asked for global_stats_1DayTimeStats, from a dict keyed by model alone -- which cannot express a name that depends on the stage. The failure this caused is quiet, which is the reason to care: a stats file that is not found is a stage that wrote none, and the diagnostics legitimately fall back to output.nc for that. An Omega run would have produced a summary with the volume-weighted columns blank and the extremes computed the coarse way, and said nothing about why. Take the name from the stage instead, via ForwardStage.stats_filename(), so it cannot disagree with what the stage told the model to write. The dict and its lookup helper go; stage_stats_path() takes the stage rather than its name, which every caller already had. Dropping the Optional return with them is not a separate change: it was only ever None for a model outside the dict, and the callers' real guard is the one that is left, that the file exists. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The test helper wrote its synthetic Omega statistics under hard-coded names -- Temperature_SpatialMax_TimeMean1Day and the rest -- into a file named from the dict this branch just removed. Both were the pre-snapshot form, so the tests agreed with each other and with nothing else. Read the names from mpaso_to_omega.yaml, the same map the code under test reads them through, and build the filename from the stage. A test that restates what the map says cannot catch the map changing. The statistics Omega does not report at all stay listed here, since that is not something the map can express: a name absent from the map and a statistic Omega has no counterpart for look identical from the outside, and only one of them should silently produce a blank column. The new test pins what went wrong rather than what broke: the path has to follow the stage's statistics interval, because Omega's name moves with it and MPAS-Ocean's does not. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The developer guide pointed at diagnostics.STATS_FILENAMES, which is gone, and described the Omega name as differing by model. It differs by stage: the analysis period is part of the name, so two stages of one sequence can write to different files. Say that, and say what the failure looks like, because it is the reason this is worth a paragraph rather than a cross-reference -- a name that is not found reads as a stage that wrote no statistics, so getting it wrong produces a thinner summary rather than an error. The rest of what this section said about Omega's statistics was already correct: the user guide's copy came from the reconciled forward branch during the rebase, and only this paragraph predated it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
hmix_ref_cell_width was the one option the 240 km entry of FORWARD_EXPECTED did not pin, and it is the option that decides whether the Compass coefficients beside it mean what they say. The mesh is quasi-uniform, so its reference is its own width; the shared default of 30 km would scale every coefficient by 240/30 while still looking like a correctly configured mesh. The existing test for this covers the variable-resolution meshes, whose reason is different -- they borrow E3SM's per-grid values, which are referenced to their finest cells. A quasi-uniform mesh has no finest cells to speak of, so it belongs here rather than there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The boundary layer, the submesoscale parameterization and the pressure gradient became config options upstream when the forward runs grew a second purpose. Because the schedule vocabulary is the ForwardStage fields, that made all three stage-settable here without a line of code. Which is exactly why they are worth a test. Nothing in this package names them, so if one were renamed or folded away upstream, the symptom would be a schedule key that quietly stopped applying -- the schedule validator would still accept it, since it would still be a field, and a schedule that had been turning KPP off would go back to leaving it on without saying so. The test also pins the defaults, because the interesting property is that a stage saying nothing keeps E3SM's physics rather than the Registry's. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The schedule-format section said a key must name a forward-run setting and left it there. That is true but unhelpful for the three options that arrived from upstream after these schedules were written: nothing points at them, and their being stage-settable is a consequence of how the vocabulary is built rather than a decision anyone recorded. Name them, say they default to E3SM's physics, and say what varying them would be for. Also say plainly that no shipped schedule does it, so a reader treats it as untried rather than as a supported idiom -- the distinction the config options themselves cannot express. The note about Omega is the part most worth having: unlike damping, which raises, these are dropped silently for Omega, because they are applied only under config_model='mpas-ocean'. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The two stage-label tests failed on Python 3.13 and passed on 3.14, with
strptime rejecting '1-01-01_00:00:00'. The helper built its start times
with strftime('%Y-...'), and whether strftime zero-pads a year below 1000
is not portable: 3.14 pads it, 3.13 leaves it to the platform. These
runs start in year 1, so the helper handed the code under test a
one-digit year that %Y then refused to parse back.
Use _format_time, which is what a schedule uses. It builds the string
with an explicit :04d rather than going through strftime, and its
docstring says "with a four-digit year" -- the production side had
already met this and avoided it.
So the test was not exercising a string any schedule can produce, which
is the actual defect here; the version split only decided whether it was
visible. Nothing in polaris/ round-trips a year through strftime, and
this was the only place in tests/ that did.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
density is in the forward output stream so that MPAS-Ocean and Omega can be compared on the same fields. A dynamic adjustment is not that comparison, and it pays for the field once per stage on the largest meshes Polaris builds: about 13% of the output volume, 2.4 GB per stage on u.oi6to18.lr6to10, eight stages deep. Nothing in this workflow reads it. The checks and the summary come from the global statistics, and the stratification questions density would answer are asked of the initial condition rather than of a stage. ssh stays -- it is 2-D, nearly free, and is the surface field worth having. This is the first time this workflow overrides a [realistic_global_forward] option in its own config rather than through a schedule or a per-mesh file, so the section says where it sits in the layering: on top of the forward config, and still under the per-mesh ones. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Checked once per mesh rather than once, because the per-mesh config files are applied after this workflow's own and could quietly turn it back on; a single-mesh test would not notice. ssh is asserted alongside it. The two sit together in the same block of the output stream, and dropping both is the obvious way to get this wrong. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The user guide embeds this workflow's config verbatim, so it carries the new section and the reason the stages drop density but keep ssh. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
xylar
force-pushed
the
add-realistic-global-ocean-dynamic-adjustment
branch
from
August 12, 2026 13:07
0be7a20 to
19ac892
Compare
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.
(More complete description to come)
This branch is based on #643
Checklist
api.md) has any new or modified class, method and/or functions listedTestingcomment in the PR documents testing used to verify the changes