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.

Sign up for GitHub

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

Jump to bottom

Introduce custom TrapezoidIntegrationCallback to integrate flows and basin forcings #1468

Closed
wants to merge 13 commits into from
Closed

Introduce custom TrapezoidIntegrationCallback to integrate flows and basin forcings #1468

wants to merge 13 commits into from

Conversation

SouthEndMusic
Copy link
Collaborator

@SouthEndMusic SouthEndMusic commented May 14, 2024 •
edited

Fixes #1473.

To do:

  • Update TrapezoidIntegrandAffect.integrand_value in update_basin! for integrands that contain the basin forcings (both for flow integration and bmi integration)
  • TrapezoidIntegrandAffect.integrand_value_prev can be removed, if the integration is done in this order:
function (affect!::TrapezoidIntegrationAffect)(integrator)::Nothing
    (; integrand_func!, integrand_value, cache, integral) = affect!
    (; dt) = integrator

    cache .= integrand_value
    integrand_func!(integrand_value, integrator.p)
    cache .+= integrand_value
    cache .*= 0.5 * dt

    integral .+= cache
    return nothing
end
  • Test the integration around a discontinuity of the basin forcings
  • Add docstrings, inline comments

@visr visr mentioned this pull request May 14, 2024
Open
@SouthEndMusic
Copy link
Collaborator Author

SouthEndMusic commented May 15, 2024 •
edited

A drawback of the current way flows and basin forcings are computed is that a vector for all these values is allocated at each timestep, while they are averaged over the saveat intervals after the simulation. It would be nice if the IntegratingCallback would support passing the saveats to the callback constructor to predefine the integration intervals, which is nice for the output flows. Maybe worth making an upstream issue.

The IntegratingSumCallback is used for the vertical fluxes for BMI, and this is cumulative and in place. An option to resolve the above is to use IntegratingSumCallback also for the above, in combination with a SavingCallback.

@SouthEndMusic
Copy link
Collaborator Author

SouthEndMusic commented May 15, 2024 •
edited

I just realized I made a wrong assumption about how IntegratingCallback and IntegratingSumCallback work. I thought that the integrand function was only evaluated at time points of the integrator, but it is in fact called at several different points:

https://github.com/SciML/DiffEqCallbacks.jl/blob/a35955b8480356503e3df492a9e02249cef00cd0/src/integrating.jl#L175C5-L177C55

The problem with this is that we don't know the value of the integrand at these in between points, at least not without calling water_balance! which is rather expensive.

@SouthEndMusic SouthEndMusic changed the title Use IntegratingCallback to integrate flows and basin forcings Introduce custom TrapezoidIntegrationCallback to integrate flows and basin forcings May 16, 2024
@SouthEndMusic
Copy link
Collaborator Author

Although this was a fun dive into creating a custom callback, it didn't reach its goal, namely reducing the complexity of the integration code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make custom trapezoidal integration callback analogous to IntegratingCallback
1 participant
@SouthEndMusic