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

Let solver integrate flows #1444

Closed
wants to merge 24 commits into from
Closed

Conversation

SouthEndMusic
Copy link
Collaborator

@SouthEndMusic SouthEndMusic commented Apr 30, 2024

Fixes #1431.

To do:

  • Let the solver integrate edge flows
  • Let the solver integrate basin forcings
  • Let the solver integrate flows for allocation input
  • Let the solver integrate realized user demand flows
  • Update Jacobian sparsity and related test
  • Docstrings etc. (maybe some parts of the documentation that now only mention the storages and the PID integral term as states)

@visr visr marked this pull request as draft April 30, 2024 16:04
@SouthEndMusic
Copy link
Collaborator Author

SouthEndMusic commented May 1, 2024

There seems to be something broken in adaptive timestepping. It looks like timesteps are allowed to be arbitrarily small (no matter the solver settings), which would explain why running the backwater model takes a ridiculous amount of time and this doesn't throw an error:

Edit: this has now been resolved. I think the state-specific tolerances in function Model and the internalnorm which only looks at the storage now aligns the solver with what we care about. The point below about the comment stil stands.

# cannot go back in time
@test_throws ErrorException BMI.update_until(model, dt0 / 2.0)

It also looks like that comment is outdated, because you don't give a negative dt but I suspect a too small one.

@visr
Copy link
Member

visr commented May 1, 2024

It also looks like that comment is outdated

It doesn't look outdated to me. First you update to t=dt0, then to t=dt2/0, which is before the current t. The SciML methods expect a dt argument, whereas BMI expects a t argument.

Comment on lines +148 to +155
# Only have finite tolerance on storage states
abstol = copy(u0)
reltol = copy(u0)
abstol .= Inf
reltol .= Inf
abstol.storage .= config.solver.abstol
reltol.storage .= config.solver.reltol

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If desired, we could apply different tolerances to different components of the states this way

@SouthEndMusic SouthEndMusic requested review from evetion and visr May 6, 2024 09:29
@SouthEndMusic SouthEndMusic marked this pull request as ready for review May 6, 2024 09:31
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.

Looks good, though the "mean_flow" test perhaps needs some double checking to see if it is ok. A flow of 0.84 where it should be 1 seems quite a large difference, though if it is only for tiny dt and evens out properly over time it should be fine.

docs/core/equations.qmd Outdated Show resolved Hide resolved
core/test/time_test.jl Outdated Show resolved Hide resolved
core/test/run_models_test.jl Outdated Show resolved Hide resolved
core/src/util.jl Outdated Show resolved Hide resolved
core/src/solve.jl Show resolved Hide resolved
core/src/util.jl Outdated Show resolved Hide resolved
core/src/sparsity.jl Outdated Show resolved Hide resolved
core/src/solve.jl Outdated Show resolved Hide resolved
Copy link
Member

@evetion evetion left a comment

Choose a reason for hiding this comment

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

Can't comment on the solver changes/setup, but overall looks ok to me, some small comments.

FWIW I would be useful for others to have a nice PR description (which turns into a commit message after (squash) merging) at least detailing what changed in the internal API for other developers who didn't see these changes.

core/src/util.jl Outdated Show resolved Hide resolved
core/src/util.jl Show resolved Hide resolved
core/src/util.jl Outdated Show resolved Hide resolved
core/src/callback.jl Show resolved Hide resolved
core/src/sparsity.jl Outdated Show resolved Hide resolved
core/src/sparsity.jl Outdated Show resolved Hide resolved
@evetion
Copy link
Member

evetion commented May 13, 2024

Also, this seems a net change in terms of lines added/removed, contrary to the original idea in #1431. Do we think this lowered our maintenance? How is the performance affected?

@SouthEndMusic
Copy link
Collaborator Author

SouthEndMusic commented May 13, 2024

So it turns out that this is pretty bad for performance, it basically negates all performance gain from #1457 as measured with the HWS model. So here's a wild idea: What if we set up a parallel ODE problem for the flow integration with an explicit solver?

@visr
Copy link
Member

visr commented May 14, 2024

Too bad this didn't work out performance wise, good effort though.

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.

Let the solver integrate our flows
3 participants