Skip to content

Insert a sentinel flowpath downstream of terminal nexuses when partitioning subset hydrofabrics (NGWPC-6553) - #32

Merged
PhilMiller merged 9 commits into
developmentfrom
PhilMiller-7218-partitioning-again
Sep 12, 2025
Merged

Insert a sentinel flowpath downstream of terminal nexuses when partitioning subset hydrofabrics (NGWPC-6553)#32
PhilMiller merged 9 commits into
developmentfrom
PhilMiller-7218-partitioning-again

Conversation

@PhilMiller

@PhilMiller PhilMiller commented Sep 10, 2025

Copy link
Copy Markdown

Prior to this PR, partitioning a hydrofabric in which there was a nexus that had multiple contributing catchments and no downstream catchment could lead to corrupted run results if those catchments were spread across multiple processes.

Fixes: NOAA-OWP#284

Additions

  • SentinelFeature type for the placeholder GeoJSON features that have no geography or geometry associated with them

Changes

  • Detect potential victim nexuses in the partitionGenerator input, and add sentinel flowpaths downstream of them
  • Elaborate the CMake summary print just a bit

Removals

  • Bits of dead code

Testing

  1. Output from partitionGenerator on test subset hydrofabric from bug report looks as intended

Screenshots

{
    "partitions":[
        {"id":0,
        "cat-ids":["cat-2395", "cat-2404", "cat-1118", "cat-758", "cat-757", "cat-1257", "cat-1344", "cat-1057", "cat-1480", "cat-1962", "cat-1368", "wb-TERMINAL_SENTINEL-nex-759"],
        "nex-ids":["nex-758", "nex-757", "nex-759"],
        "remote-connections":[
            {"mpi-rank": 1, "nex-id": "nex-759", "cat-id": "cat-1002", "cat-direction": "orig_cat-to-nex"},
            {"mpi-rank": 1, "nex-id": "nex-759", "cat-id": "cat-1143", "cat-direction": "orig_cat-to-nex"},
            {"mpi-rank": 1, "nex-id": "nex-759", "cat-id": "cat-1221", "cat-direction": "orig_cat-to-nex"},
            {"mpi-rank": 1, "nex-id": "nex-759", "cat-id": "cat-1418", "cat-direction": "orig_cat-to-nex"}]}, 
        {"id":1,
        "cat-ids":["cat-1354", "cat-1454", "cat-1591", "cat-1002", "cat-1670", "cat-1001", "cat-1143", "cat-1221", "cat-2018", "cat-1858", "cat-1418"],
        "nex-ids":["nex-1001", "nex-1002", "nex-1143", "nex-1221", "nex-1418", "nex-759"],
        "remote-connections":[
            {"mpi-rank": 0, "nex-id": "nex-759", "cat-id": "wb-TERMINAL_SENTINEL-nex-759", "cat-direction": "nex-to-dest_cat"}]}    ]
}

Checklist

  • PR has an informative and human-readable title
  • Changes are limited to a single goal (no scope creep)
  • Code can be automatically merged (no conflicts)
  • Code follows project standards (link if applicable)
  • Passes all existing automated tests
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged / future todos are captured in comments
  • Project documentation has been updated (including the "Unreleased" section of the CHANGELOG)
  • Reviewers requested with the Reviewers tool ➡️

@PhilMiller

Copy link
Copy Markdown
Author

Ok, so reading the ngen code that consumes the partitions, I think it will work fine in the presence of these added sentinels. They won't appear in the geopackage, and so they won't affect the simulation besides as communication placeholders.

@PhilMiller

Copy link
Copy Markdown
Author

Same kind of logic appears in the older GeoJSON reading code - catchments considered are the intersection of what's in the source data and what's specified as the subset from the command line or partition file.

@PhilMiller

Copy link
Copy Markdown
Author

I'm thinking that it may be necessary to add comments geo{package,json}::read and/or their call sites to note this code's reliance on them not checking that every ID named in their arguments is actually present in the data.

@PhilMiller PhilMiller changed the title Insert a sentinel tailwater flowpath downstream of terminal nexuses in subset hydrofabrics (NGWPC-6553) Insert a sentinel tailwater flowpath downstream of terminal nexuses when partitioning subset hydrofabrics (NGWPC-6553) Sep 11, 2025
@PhilMiller PhilMiller changed the title Insert a sentinel tailwater flowpath downstream of terminal nexuses when partitioning subset hydrofabrics (NGWPC-6553) Insert a sentinel flowpath downstream of terminal nexuses when partitioning subset hydrofabrics (NGWPC-6553) Sep 11, 2025
@PhilMiller

Copy link
Copy Markdown
Author

Per @zhengtaocui

I tested it with 1, 2, 4, and 6 processors. They all ran without an error. Good job!

@PhilMiller
PhilMiller marked this pull request as ready for review September 11, 2025 17:34

@cmaynard-ngwpc cmaynard-ngwpc left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Looks good. Thank you for excellent descriptions and comments.

@PhilMiller
PhilMiller merged commit dc5bdd0 into development Sep 12, 2025
9 of 10 checks passed
@PhilMiller
PhilMiller deleted the PhilMiller-7218-partitioning-again branch September 12, 2025 08:26
@cmaynard-ngwpc
cmaynard-ngwpc restored the PhilMiller-7218-partitioning-again branch September 19, 2025 23:54
@cmaynard-ngwpc
cmaynard-ngwpc deleted the PhilMiller-7218-partitioning-again branch September 26, 2025 16:12
zhengtaocui pushed a commit that referenced this pull request May 18, 2026
Insert a sentinel flowpath downstream of terminal nexuses when partitioning subset hydrofabrics (NGWPC-6553)
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.

Investigate Multiple Writes to Nexus Output Files

2 participants