Skip to content

Fix partitioning when partition boundaries fall on the edge of a domain. - #934

Merged
robertbartel merged 9 commits into
NOAA-OWP:masterfrom
hellkite500:fix-284
Feb 12, 2026
Merged

Fix partitioning when partition boundaries fall on the edge of a domain.#934
robertbartel merged 9 commits into
NOAA-OWP:masterfrom
hellkite500:fix-284

Conversation

@hellkite500

Copy link
Copy Markdown
Contributor

Cherry picked critical partitioning fix from NGWPC#32

git cherry-pick \
de5d6eb245f259319905e600d92de6f2c3836d31 \
864e0d1e64daf664d33e556bd6458b6e1ef9401a \
b0bb3b897d5b0b30880515bc7b19c01123e15d60 \
77b867e45a89d6eadf43c4183078d0e418a791bd \
23cdd4c3a1bba4d4d72219dd65cd1636349e14b1 \
523e439ad818305b2e6d5f0978cd918f35a868dd \
f00ab8566f18f48de12542b9530458bfc64ab727 \
56f4376679cae0078f76c17d1136e128146256af

With a manual rebase to include just the fixes without dependence on the not-yet-integrated logging mechanism.

Added a small fix to ensure the sentinel features don't raise errors that shouldn't be considered errors.

Addresses #284, as mentioned in the original NGWPC PR.

@robertbartel

Copy link
Copy Markdown
Contributor

@hellkite500, should we expect a different set of nexus ids? Using these changes, and testing on the NextGen_VPU_10U_v2_2.gpkg file, I see one additional (terminal) nexus present in the partition file generated: tnx-1000006101.

@robertbartel

robertbartel commented Feb 11, 2026

Copy link
Copy Markdown
Contributor

To expand on my last comment slightly, this appears to be the only terminal nexus in the hydrofabric file I used for testing that doesn't actually have any catchments going to it. So it's probably not really a problem that it is in there. It may have something to do with the nexus now having a downstream sentinel feature, though I don't immediately see where in the code that causes the difference.

@robertbartel

Copy link
Copy Markdown
Contributor

So, high-level, I think this looks good enough. I'm not yet marking it approved myself, to avoid accidentally giving the wrong at-a-glance impression of this being finished before it gets a review from those assigned, but I'm happy to come back and formally approve if you like @hellkite500.

Nit-picking a little here, but I do want to at least note that I am slightly uneasy with this approach as a permanent solution. Least importantly, it adds noise to the partition config, with a lot sentinel features not even involved in remote connections. Not unrelated, it also seems (granted, with limited time spent analyzing other solution options) more like a workaround than something addressing the real issue. And it introduces an inconsistency between the ngen and hydrofabric data models. This inconsistency currently has no side effects, but that is incidental to the current implementation and not necessarily to the underlying problem.

@PhilMiller

Copy link
Copy Markdown
Contributor

It's absolutely a workaround, to ensure that the generated partitions satisfy the assumptions made in ngen's parallelism logic. I would have much rather addressed this inside ngen, but that posed much higher hurdles. There were expedient NGWPC needs to get things running at all that led to me taking this approach.

@robertbartel robertbartel left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Turns out no reviewers were actually assigned as I previously thought (only suggested), so I'm going to go ahead and approve this.

@robertbartel
robertbartel merged commit de28c2a into NOAA-OWP:master Feb 12, 2026
21 checks passed
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.

3 participants