Skip to content

Stream Flow Routing (SFR) #1497

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

Stream Flow Routing (SFR) #1497

wants to merge 4 commits into from

Conversation

Huite
Copy link
Contributor

@Huite Huite commented Mar 26, 2025

Still work in progress, but adds most of the required logic for an SFR package.

This requires an unreleased xugrid Ugrid1d method, but otherwise it looks like the Ugrid1d class suffices.

One of the main things that requires a decision is how to assign the network to layers. Unlike a river or drainage boundary, it doesn't seem like a great idea idea to assign a single water body to multiple cells (even if intersects multiple layers), because the SFR package computes a single water height per reach.

I think the easiest solution is to add a layer variable. If this isn't provided (None), I guess we'd use the streambed elevation to assign to a layer instead in combination with DIS top and bottom data.

It would also be good to have another critical look at the AdvancedBoundary class. It looks like they have very little in common, so we should probably just make that an abstract base class which requires the write_blockfile method, but not much else. Also worth researching is where external files are allowed (which apparently works for UZF?). This just dumps to the blockfile, like the lake package implementation.

The melting approach for the period data works to some satisfaction, it looks better to me than the current period jinja stuff in the lake package.

@Huite Huite marked this pull request as draft March 26, 2025 14:20
@Huite Huite requested a review from JoerivanEngelen March 26, 2025 14:20
@Huite
Copy link
Contributor Author

Huite commented Mar 27, 2025

I've created a basic example, which seems to give reasonable results (I've only looked at heads, though).

The most controversial thing so far is that just like the HFB and Well classes, this is a grid agnostic package. So it needs information from DIS (a dst_grid) to place the elements. Placing the elements can be done in a simple way, by simply placing them where the edge centroid is located (alternatively grid cell with largest overlap). (Note that an SFR reach can only be connected to one cell at most, subdivision should be done via regridding.) Anyway, I've been lazy so far, and I just added a cellid variable to the existing class so I don't have to duplicate everything.
Having a separate class is probably best, because it's what you'd want to represent e.g. bare input as well (without the network topology).

Some tedious things:

For a next step, I'll see whether the budget output can be parsed somewhat effectively.
Ideally, it's placed on the network topology again.

@Huite
Copy link
Contributor Author

Huite commented Mar 28, 2025

I've added some basic functions to read the output stages and budgets as well.

There is clear opportunity here to refactor the output reading stuff. What makes things a bit more complicated:

  • We have data that makes sense on the model grid. This is true for most packages, like NPF flows, but also in how we treat rivers, drainage, etc.
  • This is also true for the UZF budget logic that @HendrikKok added/had to make slight modifications for.
  • In principle, the SFR data is valuable on both the grid (summed values for reaches), but also on the original network topology.
  • A similar argument applies to multi-aquifer-wells (MAW) which you'd probably like to get as point data too.

I originally chose some dict based dispatch:

_READ_GRB = {
    "grid dis": dis.read_grb,
    "grid disv": disv.read_grb,
    "grid disu": disu.read_grb,
}

_OPEN_HDS = {
    "dis": dis.open_hds,
    "disv": disv.open_hds,
    "disu": disu.open_hds,
}

_OPEN_CBC: Dict[str, Callable] = {
    "dis": dis.open_cbc,
    "disv": disv.open_cbc,
    "disu": disu.open_cbc,
}

_OPEN_DVS = {
    "dis": dis.open_dvs,
    "disv": disv.open_dvs,
    "disu": disu.open_dvs,
}

I think this should be simply replaced by objects and inheritance, letting Python do the method dispatch in the regular manner.

We could then have separate readers; one for DIS, DISV, DISU, SFR, etc.

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.

1 participant