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 flow limiter (step_limiter!) #1904

Closed
wants to merge 2 commits into from
Closed

Add flow limiter (step_limiter!) #1904

wants to merge 2 commits into from

Conversation

SouthEndMusic
Copy link
Collaborator

@SouthEndMusic SouthEndMusic commented Oct 15, 2024

Fixes #1900.

Drainage infiltration MWE

Main branch

plot

This branch

plot

Conclusions

Having zero forcing when zero forcing is expected is now enforced properly. In the same way, I'm pretty sure forcing will now never exceed the given flux, and flow should be non-negative where expected.

Having total forcing when the basin is not in the low storage factor regime is harder to enforce, because there is no exact way to infer whether the basin storage passed through the reduction factor regime in the last timestep. A conservative heuristic might suffice here though.

Performance comparisons

Main branch

  • HWS: 9.837521 seconds
  • De Dommel: 51.496409 seconds

this branch

  • HWS: 9.554516 seconds
  • De Dommel: 64.802680 seconds

@SouthEndMusic SouthEndMusic marked this pull request as draft October 15, 2024 09:20
@evetion
Copy link
Member

evetion commented Oct 15, 2024

This is a fix for #1900, not #1897 right?

@SouthEndMusic
Copy link
Collaborator Author

SouthEndMusic commented Oct 15, 2024

It is indeed a fix for #1900, which is one way in which the flows have to be 'tamed' as discussed in #1897. Even though it is not a global fix, I think this PR is a helpful one, as it enforces properties of the solution which we want to be strictly true for any set of solver settings, e.g.

  • Non-negative flows where expected
  • Realized user demand should not exceed demand

With the ever increasing norm of the state there is still the possibility that the solution slowly drifts away from the physics in ways we cannot capture in the step_limiter. If we want to solve that and keep using the fast solver QNDF, we either have to find a way to tame the state size (for instance getting resetting to 0 to work), or document that longer simulations require a smaller relative error tolerance.

@SouthEndMusic SouthEndMusic marked this pull request as ready for review October 16, 2024 07:55
@@ -718,3 +699,114 @@ function formulate_flows!(
formulate_flow!(du, user_demand, p, t, current_storage, current_level)
end
end

"""
Clamp the cumulative flow states between bounds given my minimum and maximum
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Clamp the cumulative flow states between bounds given my minimum and maximum
Clamp the cumulative flow states between within the minimum and maximum

allocation,
) = p

# TabulatedRatingCurve
Copy link
Member

Choose a reason for hiding this comment

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

It would be good to also describe what/how/why is being limited here. In this case, we know that a TaublatedRatingCurve has a flow bounded by [0.0, Inf] and has an active component?

)
end

# Pump
Copy link
Member

Choose a reason for hiding this comment

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

This one requires less documentation, because it uses the Pump properties directly?

@SouthEndMusic SouthEndMusic marked this pull request as draft October 19, 2024 14:56
@SouthEndMusic
Copy link
Collaborator Author

I've added the Average flow states commit for the record since I was working on it on this branch, but I'm not going to use it. I feel like it's not worth the added complexity, both conceptually and in the code. My goal now is to fix urgent things with the step limiter, and either stop using the relative error (i.e. hardcoding it to be 0) or documenting that it should be very small if users note growing inaccuracies for long simulations.

@SouthEndMusic
Copy link
Collaborator Author

Closed in favor of #1911.

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.

Avoiding negative flows where they should not be
2 participants