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 wrapper to hide dss internals #293

Open
juliasloan25 opened this issue Aug 8, 2023 · 6 comments · May be fixed by #321
Open

add wrapper to hide dss internals #293

juliasloan25 opened this issue Aug 8, 2023 · 6 comments · May be fixed by #321
Labels
enhancement New feature or request

Comments

@juliasloan25
Copy link
Member

Is your feature request related to a problem? Please describe.
Currently, when a user is running a mixed implicit/explicit simulation, we require them to construct a ClimaTimeSteppers ClimaODEFunction manually. Part of this step involves passing in the dss function (see here). Users may not know what dss is, and we want them to be able to use our model without worrying about it.

To solve this, we can create a wrapper function (e.g. make_ode_function), which takes in the tendencies, jacobian update function, and Y, and constructs the jac_kwargs as well as the ClimaODEFunction. This will isolate the usage of our ClimaLSM.dss! function and make the code more user-friendly.

cc @kmdeck

@juliasloan25 juliasloan25 added the enhancement New feature or request label Aug 8, 2023
@kmdeck
Copy link
Collaborator

kmdeck commented Sep 5, 2023

I played around with this a little this morning...we can't implement this without adding both ClimaTimesteppers and ODE.jl to the ClimaLSM main project dependencies. This is because, in the implicit model case, we have

  CTS.ClimaODEFunction(
            T_exp! = exp_tendency!,
            T_imp! = ODE.ODEFunction(imp_tendency!; jac_kwargs...),
            dss! = ClimaLSM.dss!,
        ),

I dont mind adding CTS, but ODE.jl really slows down the precompilation time.

What do you think? Maybe we can wait on this and for now just add clear documentation that dss! must be used (and why)?

@kmdeck
Copy link
Collaborator

kmdeck commented Sep 5, 2023

For now, I will make a PR that adds dss! to all simulations/tutorials and notes that we need to use it.
#321

@kmdeck kmdeck linked a pull request Sep 5, 2023 that will close this issue
1 task
@juliasloan25
Copy link
Member Author

Charlie recommended that we could avoid adding ODE.jl by using SciMLBase.jl instead. SciMLBase implements ODEFunction, ODEProblem, and solve, which is what we're currently using ODE for. I like this as a solution to avoid longer precompilation due to ODE.jl, in src/ but also in all of our other directories. Here is a PR in ClimaAtmos.jl attempting to make this change there: CliMA/ClimaAtmos.jl#2074. They ran into some trouble because they're using some functions that are defined in ODE.jl, but we don't have this problem, so it should be a pretty straightforward change for us.

As a note: I noticed that since we now have DiffEqCallbacks in the top-level project, ODE.jl appears in the Manifest under [deps.DiffEqCallbacks.weakdeps]. I thought this might already affect the precompilation time even though ODE.jl is not a direct dependency, but Charlie explained that a weakdep will not get loaded automatically and therefore not affect precompilation unless it's explicitly loaded (just wanted to document that here).

@juliasloan25
Copy link
Member Author

Charlie just opened a PR making this fix: #323

@kmdeck
Copy link
Collaborator

kmdeck commented Sep 7, 2023

awesome! Let's do something like...merge your safe test PR #322 and then we merge #323 , and then go back to the dss PR I had 🔢 #321 - we can either rework it to add in the wrapper function or else close it and start fresh.

@kmdeck
Copy link
Collaborator

kmdeck commented Sep 7, 2023

I like this as a solution to avoid longer precompilation due to ODE.jl, in src/ but also in all of our other directories.

good idea!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants