Skip to content
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

Combined vertical fluxes on Basin #661

Closed
visr opened this issue Oct 10, 2023 · 5 comments · Fixed by #1300
Closed

Combined vertical fluxes on Basin #661

visr opened this issue Oct 10, 2023 · 5 comments · Fixed by #1300
Assignees
Labels
modflow issues related to coupling modflow waterquality Issues related to Delwaq coupling/functionality

Comments

@visr
Copy link
Member

visr commented Oct 10, 2023

One limitation of the way we save flows in #644 is that we cannot separate the 4 basin fluxes (precipitation, evaporation, drainage, infiltration). We could make separate accommodations for those. Or perhaps these should really be separate node types with 1 flux each;

  • Precipitation
  • Evaporation (specify potential_evaporation and connect to Basin to get open water evaporation)
  • Infiltration (combine with drainage? one node per modflow cell?)

This would also fix the timeseries duplication problem. Say you have 1000 Basins in a small area with a single precipitation timeseries. Now you have to copy this data 1000x, once per Basin. If you have a separate Precipitation node, you only have to define it once and connect it to all 1000 Basins, just like you can do already for e.g. a LevelBoundary. For visualization you'd probably want an easy way to hide these nodes and edges though.

Originally posted by @visr in #644 (comment)

@gijsber
Copy link
Contributor

gijsber commented Oct 10, 2023

From a modeller's perspective I am not fond of this solution as it involves another connection to track that your timesries ends up in the correct basin. But if we can hide it from the modeller and julia or ribasim_python offers the methods to post/get these series using the basinId it probably is ok.

@visr
Copy link
Member Author

visr commented Oct 10, 2023

Yes I think it should be straightforward to create tooling to follow the indirection.

@SnippenE SnippenE added the allocation Allocation layer label Oct 10, 2023
@SouthEndMusic SouthEndMusic removed the allocation Allocation layer label Nov 10, 2023
@SnippenE SnippenE added the modflow issues related to coupling modflow label Dec 7, 2023
@visr visr changed the title combined vertical fluxes on Basin Combined vertical fluxes on Basin Feb 6, 2024
@visr visr added the waterquality Issues related to Delwaq coupling/functionality label Feb 6, 2024
@Huite
Copy link
Contributor

Huite commented Feb 15, 2024

I guess the benefit additional nodes is that you can always store a "flow" variable for a given node, so all the information is encoded in the network and the types. I'm not too fond it for the same reason @gijsber mentions, but also conceptually: I've always considered the nodes to be somewhat physically distinct entities, which is reflected by the network topology.

I would vote for what feels like the solution here: treat the edge flows separately in the output. I find it a bit clunky in how you need to select basin flows anyway. Why not store a separate table with a basin index and the various balance terms for that timestep?

MODFLOW 6 also has different format for internodal flows (flow-ja-face) and flows to the node itself.

In UGRID terms, similarly: the internodal flows would be stored on the edges. The node terms would be stored on the nodes.
Although in case of UGRID, you wouldn't be able to combine it nicely either, because the nodes of the topology are so dissimilar (a control node obviously has no precipitation term).
So in the case of writing to UGRID, you either need a lot of padding values (NaN for the precipitation variable of the control nodes e.g.), or you need to define separate dimensions per type of node. Incidentally, the padding approach is what we do for the MODFLOW 6 boundary budgets. In that case, it's well worth it IMO, because it makes it a lot easier for the user to interact with the data: they need only know about DataArrays which represent the entire topology. This ease of interaction easily wins out over the somewhat wasteful duplication caused by padding NaNs everywhere. It means you can directly make spatial selections and easily visualize the data. QGIS also directly understands the data in that form.

@SnippenE SnippenE added needs-refinement Issues that are too large and need refinement and removed needs-refinement Issues that are too large and need refinement labels Feb 22, 2024
@visr
Copy link
Member Author

visr commented Feb 22, 2024

We won't go with separate nodes to split combined flows, instead we remove the combined flows and make sure we add separate fluxes to the node specific output files.

@elinenauta elinenauta assigned elinenauta and visr and unassigned elinenauta Mar 14, 2024
@evetion
Copy link
Member

evetion commented Mar 14, 2024

Depending on functionality that gets removed (like flow edges without an id that can be recovered from the network), introduce an utility that recover them in this issue.

visr added a commit that referenced this issue Mar 28, 2024
Fixes #661.

---------

Co-authored-by: Martijn Visser <mgvisser@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
modflow issues related to coupling modflow waterquality Issues related to Delwaq coupling/functionality
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

7 participants