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

Refactor flow integration #1735

Closed
wants to merge 30 commits into from
Closed

Conversation

SouthEndMusic
Copy link
Collaborator

@SouthEndMusic SouthEndMusic commented Aug 19, 2024

The flow integration is now refactored so that integrated flows are computed based on water_balance! calls after the timestep but before parameter updates, so that the flows are calculated with the values of the discrete parameters that they had over the last timestep.

I also made sure that the db is closed properly in the tests.

This PR was originally intended to fix #1413, but that was put on hold because the resulting water balance errors were quite large, which will possibly be fixed by #1808.

@SouthEndMusic SouthEndMusic marked this pull request as draft August 19, 2024 12:57
@SouthEndMusic
Copy link
Collaborator Author

TL;DR for @HendrikKok : there are probably still problems with flow integration in the current Ribasim main.

In the interval of a single timestep, discrete parameters (from DiscreteControl, basin forcings) are fixed. That means that for accurate flow integration over that interval, all flows must be formulated with these correct parameters set. This is particularly important on the boundary point with the previous timestep, which we currently do wrong on main. To solve this systematically, flow integration should only happen with flows formulated in the single integrate_flows! call, which happens after a timestep and before the updates of discrete parameters for the next timestep.

I already know this isn't the holy grail for fixing the water balance error computations, because I experimented with integration schemes that satisfy the above and the large water balance errors are still there.

@SouthEndMusic SouthEndMusic changed the title Error on large water balance error Refactor flow integration Sep 9, 2024
@SouthEndMusic SouthEndMusic marked this pull request as ready for review September 9, 2024 12:33
@SouthEndMusic SouthEndMusic requested review from visr and removed request for visr September 9, 2024 12:33
@SouthEndMusic SouthEndMusic marked this pull request as draft September 9, 2024 19:13
@SouthEndMusic
Copy link
Collaborator Author

This has been made redundant by #1819.

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.

Error on large water balance error
1 participant