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

use dss in ClimaODEfunction; update docs #321

Closed
wants to merge 5 commits into from
Closed

use dss in ClimaODEfunction; update docs #321

wants to merge 5 commits into from

Conversation

kmdeck
Copy link
Member

@kmdeck kmdeck commented Sep 5, 2023

Purpose

Closes #293.
Hides some internals from users with respect to setting up the ClimaODEFunction:
-Employs dss! in all tutorials and experiments. internally, whether or not dss! does anything depends on the domain of the model/whether spectral elements are used.
-creates the function get_ClimaODEFunction(model, Y) which creates the clima ode function suitable for the model (implicit/explicit tendencies, jacobian, dss)

To-do

  • Make use of AbstractImExModel and AbstractExpModel types to create two methods for:
    get_ClimaODEFunction(model, Y) (or do we need to? does it matter if the jacobian is nothing, the update_jacobian function does nothing?)
  • Define a method of get_ClimaODEFunction(model, Y) for AbstractLandModels which constructs a jacobian for the entire model out of the jacobian of subcomponents (future PR?)

This arises because we have:
AbstractLandModel <: AbstractModel
AbstractImExModel <: AbstractModel
AbstractExpModel <: AbstractModel

currently in this PR, we only have the single method for AbstractModel for all jacobian functions, and for the get_ClimaODEFunction.

Make clear in tutorials when to use explicit steppers vs imex timesteppers. Right now this has to be inferred from the type of the model or type of the component models of a land model.

Content

Creates get_ClimaODEFunction, which required a function make_jacobian, and uses this is all tutorials/experiments

Review checklist

I have:

In the Content, I have included

  • relevant unit tests, and integration tests,
  • appropriate docstrings on all functions, structs, and modules, and included relevant documentation.

  • I have read and checked the items on the review checklist.

@juliasloan25
Copy link
Member

After #323, we can make a dss wrapper function without introducing a dependency on ODE.jl, just SciMLBase.jl. Maybe we should do that instead as a more comphrehensive solution to #293?

Update richards_equation.jl

remove spurious comma
@kmdeck kmdeck marked this pull request as draft September 13, 2023 20:54
@juliasloan25
Copy link
Member

@kmdeck I think we can merge this (or something like it) now. I forget what state this is in and why we didn't merge it... Does it just need a review?

@kmdeck
Copy link
Member Author

kmdeck commented Dec 18, 2023

@kmdeck I think we can merge this (or something like it) now. I forget what state this is in and why we didn't merge it... Does it just need a review?

I will rebase & revisit! I forget too.

@Sbozzolo
Copy link
Member

Sbozzolo commented Jul 2, 2024

@kmdeck maybe we can close this now?

@kmdeck
Copy link
Member Author

kmdeck commented Jul 2, 2024

@kmdeck maybe we can close this now?

Yes, I think we can!

closing since we have currently removed dss! from our simulations (partially, the infrastructure is still there). If we do bring it back, and even if not, I think it would be nice to have a helper function to create the ClimaODEFunction. but we can start that fresh. this PR is very stale.

@kmdeck kmdeck closed this Jul 2, 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.

add wrapper to hide dss internals
4 participants