Add interior/custom OBC segment support - #269
Open
manishvenu wants to merge 15 commits into
Open
manishvenu wants to merge 15 commits into
manishvenu wants to merge 15 commits into
Conversation
Ports manish/interior-obc-segment onto class-merge's forcing/ layout, rather than rebasing its history directly (its own ancestor predates the whole forcing_configurations+extract_forcings merge, and half its diff was already carried in independently via cice-as-reference during refactor-obc-ic). Bumps rm6 further (to the current tip of the standalone regional-mom6 repo's manish/standalone-boundary-class branch) to pick up its cleaned-up Segment class (ocean_side replacing mom6_index_reverse, land-capped-endpoint checks) -- migrates forcing/mom6.py's OBC regrid step and forcing/tides.py's tidal regrid off the old rm6.segment(...) factory/experiment.setup_boundary_tides onto Segment.cardinal/from_spec and Segment.regrid_tides directly. A boundary can now be a live regional_mom6.segment.Segment (built via Segment.from_hgrid/from_lonlat) instead of just a cardinal string -- ConditionsConfigurator/TidesConfigurator normalize it to a boundary-key string for JSON safety and carry its full spec separately in config.json's custom_segments, threaded through obc.py's GET-step bbox lookup and mom6.py's regrid step (bound in via functools.partial, keeping obc.py's regrid_chunk_fn contract unchanged for cice/ww3). Case.configure_forcings's boundaries default changes from a hardcoded 4-cardinal list to None, which auto-detects the open cardinal edges from ocn_topo via Segment.detect_open_cardinal_boundaries. Also removes Case.expt, an unused property (only referenced by one test assertion) whose hardcoded cardinal-only boundary handling doesn't generalize to custom segments -- this also happens to remove one of the sandbox's known RUNDIR-creation flaky tests.
4 tasks
Codecov Report❌ Patch coverage is
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
|
📄 Preview your docs here: |
5 tasks
Contributor
|
@manishvenu Could you merge the latest main, resolve conflicts, and make sure to not drop any bugfixes from the latest main? |
…anch with the cholrophull stuff
test_regrid_obc_chunk_with_reference_ocean is the first test in the suite to touch ESMF, and it killed pytest outright on the runner -- no traceback, no test verdict, just "Process completed with exit code 15". The env resolved esmf 8.6.1 mpi_mpich_*, whose MPI_Init on first use intermittently aborts on GitHub runners. CrocoDash only uses ESMF serially via xesmf, so the MPI stack buys us nothing here. Same fix as NCAR/mom6_forge#107, plus one thing that repo didn't need: create-args is not part of cache-environment-key, so without bumping the key prefix the job would restore the cached MPI env and the pin would silently do nothing. Scoped to the workflow -- environment.yml is untouched, so dev envs are unaffected.
Member
Author
|
@alperaltuntas This should be merged up from main. |
Brings in class-merge's merge of main (#265 squash-merged refactor-obc-ic). Two conflicts, both resolved in favour of this branch: - forcing/mom6.py's _regrid_obc_chunk. class-merge migrates it to rm6's standalone Segment class; this branch had already done that migration and generalised past it, so ours is the superset: * get_segment() over Segment.cardinal() -- get_segment dispatches to Segment.cardinal for a cardinal boundary and to Segment.from_spec for a custom/interior one, which cardinal-only cannot express. * class-merge's incoming repeat_year_forcing=False would have been a DUPLICATE keyword -- this branch already passes it further down the same regrid_velocity_tracers() call. Taking it would not have merged badly, it would have been a SyntaxError. Net result: mom6.py is unchanged from this branch's HEAD. The merge also brought a second `from regional_mom6.segment import Segment` on top of the one already here; dropped the duplicate, kept ours (Segment is still used directly by the isinstance check in configure). - CrocoDash/rm6: 1aa7c43d (temp_seg_with_chl_branch) vs main's 23b71bc5. Resolved to 23b71bc5. Their trees are byte-identical -- the standalone Segment work reached main via the squash of rm6 #315 -- so this is a lineage fix, not a content change, and it puts this branch on main's rm6. visualCaseGen cd8e95fa -> bff27e64 and the refactor-obc-ic IC/OBC coverage tests merged clean. Verified both test files parse with no duplicate test names, and black is clean.
The class-merge merge took that branch's copy of
test_process_bc_hands_the_engine_mom6s_own_pieces, which asserts the engine
is handed _regrid_obc_chunk itself:
assert functools.partial(_regrid_obc_chunk, custom_segments={})
is _regrid_obc_chunk
On this branch it never is. process_bc binds custom_segments onto the chunk
regridder first (mom6.py), because an interior boundary cannot be rebuilt
from its name alone the way a cardinal one can -- so what reaches the engine
is a partial. The pre-merge test file knew that; the merge replaced it with
one written against cardinal-only boundaries.
Assert on the partial instead: that it wraps _regrid_obc_chunk, and that
custom_segments is what got bound.
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
2 participants
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.
Descrip:
Adds interior/custom OBC segment support on the merged
forcing/pattern from #265/#266, replacing #256. A boundary can now be a liveregional_mom6.segment.Segment(built viaSegment.from_hgrid/from_lonlat), not just a cardinal string.Changes:
Bumps
rm6to the current tip of the standaloneregional-mom6repo'sSegmentclass (ocean_sidereplacingmom6_index_reverse, land-capped-endpoint checks), and migrates OBC regrid and tidal regrid off the oldrm6.segment(...)factory /experiment.setup_boundary_tidesontoSegment.cardinal/from_spec/regrid_tidesdirectly.Case.configure_forcings(boundaries=None)now auto-detects open cardinal edges fromocn_topoinstead of hardcoding all four.Minor changes:
Case.exptproperty — cardinal-only, doesn't generalize to custom segments.