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

Fail water balance check on either tolerance #1858

Closed
wants to merge 1 commit into from
Closed

Fail water balance check on either tolerance #1858

wants to merge 1 commit into from

Conversation

visr
Copy link
Member

@visr visr commented Oct 1, 2024

@evetion
Copy link
Member

evetion commented Oct 1, 2024

Good luck with finding new tolerances, nice PR just before 1.0! ❤️

@visr
Copy link
Member Author

visr commented Oct 1, 2024

Doesn't need to be merged before 1.0. Could have if it didn't give issues.
This is on pump_discrete_control, relative error is too high, but absolute error is tiny:

┌ Error: Too large water balance error
│   id = Basin #1
│   balance_error = -4.1119371282413245e-20
│   relative_error = -0.10526315789473695
└ @ Ribasim ~/work/Ribasim/Ribasim/core/src/callback.jl:316

@visr visr marked this pull request as draft October 1, 2024 09:16
@Huite
Copy link
Contributor

Huite commented Oct 1, 2024

Uh, oops, I might've been shooting from the hip there. I was thinking of acceptance rather than rejection!

What I discussed with @SouthEndMusic earlier: if the absolute error is tiny, you probably want to accept, right?
Hence the earlier implementation. Since && short-circuits, it won't look at the relative error if the absolute error is already okay.

@visr visr closed this Oct 1, 2024
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