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

Add remaining water balance terms to basin.arrow #1367

Merged
merged 6 commits into from
Apr 9, 2024
Merged

Add remaining water balance terms to basin.arrow #1367

merged 6 commits into from
Apr 9, 2024

Conversation

visr
Copy link
Member

@visr visr commented Apr 8, 2024

Technically these terms can be calculated from flow.arrow and the storage. But since calculating the water balance for a Basin is such a common operation, we include them directly. This also makes it easier to locate water balance errors in time and space, by including the error term.

At some point we may also want to add the instantaneous inflow and outflow to the Basin struct, and update them in formulate_du!. This would allow us to reject steps with too large of a balance error, using one of the metrics mentioned in #1271. But for now this only adds them to the output via a callback.

Technically these terms can be calculated from flow.arrow and the storage. But since calculating the water balance for a Basin is such a common operation, we include them directly. This also makes it easier to locate water balance errors in time and space, by including the error term.

At some point we may also want to add the instantaneous inflow and outflow to the Basin struct, and update them in `formulate_du!`. This would allow us to reject steps with too large of a balance error, using one of the metrics mentioned in #1271. But for now this only adds them to the output via a callback.
@visr visr requested a review from SouthEndMusic April 8, 2024 20:57
@visr
Copy link
Member Author

visr commented Apr 8, 2024

@gijsber @DanielTollenaar does this proposal and the change in usage.qmd look ok to you?

Copy link
Collaborator

@SouthEndMusic SouthEndMusic left a comment

Choose a reason for hiding this comment

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

Good stuff

core/src/write.jl Outdated Show resolved Hide resolved
docs/core/usage.qmd Outdated Show resolved Hide resolved
@SouthEndMusic
Copy link
Collaborator

At some point we may also want to add the instantaneous inflow and outflow to the Basin struct, and update them in formulate_du!. This would allow us to reject steps with too large of a balance error, using one of the metrics mentioned in #1271. But for now this only adds them to the output via a callback.

Would it be an idea to integrate the absolute water balance error per basin as a state of the system of differential equations, and throw an error in the same way we do for negative storages if this gets too big? Now that I think of it, why don't we integrate the vertical fluxes this way, just let the solver do it?

@DanielTollenaar
Copy link
Contributor

Good stuff! What about considering a relative_error as well?

How I compute these in Python (think we have to think about something to avoid replace infinite (balance error devided by 0 in- or outflow_rate):

# possible positive error (if error not 0): inflow is larger than outflow
positive = balance_error_df["inflow_rate"] > balance_error_df["outflow_rate"]
 
# if positive error, we devide the error by the outflow_rate (x% too much inflow compared to outflow, or x% too little outflow compared to inflow)
balance_error_df.loc[positive, ["relative_error"]] = (
    balance_error_df[positive]["error"] / balance_error_df[positive]["outflow_rate"]
) * 100
 
# if negative error, we devide the error by the inflow_rate (x% too much outflow compared to inflow, or x% too little inflow compared to outflow)
balance_error_df.loc[~positive, ["relative_error"]] = (
    balance_error_df[~positive]["error"] / balance_error_df[~positive]["inflow_rate"]
) * 100

@SouthEndMusic, guess relative_error is something you may use in the solver?

@visr
Copy link
Member Author

visr commented Apr 9, 2024

In 12c77be I renamed error to balance_error and added relative_error, as a fraction, not a percentage.

@DanielTollenaar I added your relative_error, though not sure if this is a good general metric. I agree that it is useful for Basins that are in a river with both inflow and outflow. But now if either is 0 then the relative_error is 0. Why this particular metric? Perhaps a fraction discrepancy of all inflows (horizontal and vertical) compared to all outflows is more useful. I think this is more similar to the MODFLOW output as posted at the bottom of #642 (comment).

@SouthEndMusic I think it is a good idea to let the integrator integrate the flows as well. Do you want to create an issue for it? We should probably do a small test first to get a feeling for it.

@DanielTollenaar
Copy link
Contributor

DanielTollenaar commented Apr 9, 2024

In 12c77be I renamed error to balance_error and added relative_error, as a fraction, not a percentage.

@DanielTollenaar I added your relative_error, though not sure if this is a good general metric. I agree that it is useful for Basins that are in a river with both inflow and outflow. But now if either is 0 then the relative_error is 0. Why this particular metric? Perhaps a fraction discrepancy of all inflows (horizontal and vertical) compared to all outflows is more useful. I think this is more similar to the MODFLOW output as posted at the bottom of #642 (comment).

@SouthEndMusic I think it is a good idea to let the integrator integrate the flows as well. Do you want to create an issue for it? We should probably do a small test first to get a feeling for it.

#metric all inflows; agreed, that's better.

Copy link
Collaborator

@SouthEndMusic SouthEndMusic left a comment

Choose a reason for hiding this comment

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

Added some final comments

core/src/write.jl Outdated Show resolved Hide resolved
core/src/write.jl Outdated Show resolved Hide resolved
core/src/write.jl Outdated Show resolved Hide resolved
visr and others added 3 commits April 9, 2024 14:17
Co-authored-by: Bart de Koning <74617371+SouthEndMusic@users.noreply.github.com>
@visr visr merged commit 5cf4492 into main Apr 9, 2024
24 checks passed
@visr visr deleted the basin-arrow branch April 9, 2024 13:00
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.

None yet

3 participants