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

Update reduction factors of resistance nodes #1796

Merged
merged 5 commits into from
Sep 4, 2024
Merged

Conversation

SouthEndMusic
Copy link
Collaborator

@SouthEndMusic SouthEndMusic commented Sep 4, 2024

These changes are needed for running the De Dommel model for Ribasim NL and were requested by Neeltje. The main change is that ManningResistance has low storage factors now, which is important to have if the connected basins have different bottom levels.

I made an unified approach for low storage factors of LinearResistance and ManningResistance, where the former already had low storage factors for both connected basins. That however leads to weird behavior where water cannot flow into an empty basin via the resistance node, so now I made it so that the reduction factor is based on the storage of the upstream basin as defined by the flow direction. This feels a bit discontinuous but I think it's fine.

I also fixed an upstream issue in FindFirstFunctions, for now the manifest points at the branch with the fix: SciML/FindFirstFunctions.jl#26.

@visr
Copy link
Member

visr commented Sep 4, 2024

The Ribasim builds fail with

WARNING: Method definition (::FindFirstFunctions.Guesser{T<:(AbstractArray{T, 1} where T)})(Any) in module FindFirstFunctions at C:\Users\svc-teamcity-ansible\.julia\packages\FindFirstFunctions\2w5qZ\src\FindFirstFunctions.jl:233 overwritten in module Ribasim at D:\buildAgent\work\ecd2b8f9b25b1609\ribasim\core\src\util.jl:69.
09:53:18   ERROR: Method overwriting is not permitted during Module precompilation. Use `__precompile__(false)` to opt-out of precompilation.

Looking at https://discourse.julialang.org/t/error-method-overwriting-is-not-permitted-during-module-precompilation-use-precompile-false-to-opt-out-of-precompilation/109191, you can try out if we can override from __init__? Possibly you need eval for that. Or is there another way to work around this without overwriting methods?

Or we could just point the Manifest.toml to your branch of FindFirstFunctions, perhaps that is easiest.

@visr
Copy link
Member

visr commented Sep 4, 2024

With my previous comment and a unit test of low_storage_factor_resistance_node this should be good to go, nice find!

Copy link
Member

@visr visr left a comment

Choose a reason for hiding this comment

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

I see your PR just got merged as well

@SouthEndMusic SouthEndMusic merged commit f0a8747 into main Sep 4, 2024
26 of 27 checks passed
@SouthEndMusic SouthEndMusic deleted the reduce_manning branch September 4, 2024 12:29
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.

2 participants