Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

simplify imex ark #212

Closed
wants to merge 11 commits into from
Closed

simplify imex ark #212

wants to merge 11 commits into from

Conversation

simonbyrne
Copy link
Member

Purpose

To-do

Content


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

bors bot added a commit that referenced this pull request Sep 21, 2023
210: Add post callbacks r=charleskawczynski a=charleskawczynski

This PR adds `post_explicit!` and `post_implicit!` callback hooks to ClimaODEFunction, which do nothing by default, and calls to functions were added in the ARS343`step_u!`. Should we bother calling these functions in the generic callback? cc `@simonbyrne` 

I spoke with `@simonbyrne` about this, and he'll add these into #212 once we are happy with a version that is simpler to reason about.

Co-authored-by: Charles Kawczynski <kawczynski.charles@gmail.com>
Comment on lines +39 to +41
T_lim = ntuple(i -> zero(u0), Nstage_exp)
T_exp = ntuple(i -> zero(u0), Nstage_exp)
T_imp = ntuple(i -> zero(u0), Nstage_imp)
Copy link
Member

Choose a reason for hiding this comment

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

Just a heads up: we previously had tuples and this resulted in massive compilation times (especially for the edmf cases). That's why we collect above.

Copy link
Member

Choose a reason for hiding this comment

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

#88

Copy link
Member Author

Choose a reason for hiding this comment

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

so you use vectors?

Copy link
Member Author

Choose a reason for hiding this comment

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

that's probably the same issue as CliMA/ClimaCore.jl#1467

Copy link
Member

@charleskawczynski charleskawczynski Sep 21, 2023

Choose a reason for hiding this comment

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

so you use vectors?

Yes

that's probably the same issue as CliMA/ClimaCore.jl#1467

That seems likely but considering how many fields we have in the solver cache, it's a bit strange that this ntuple, which only has a few, was so expensive. Maybe we were getting hit by more things that operate on the state?

@charleskawczynski
Copy link
Member

Can/should we close this? ( it should probably at least be rebased )

@charleskawczynski
Copy link
Member

I like the idea of simplifying along this direction, but I see a logical bug:T_lim! is called (which is supposed to compute the limited tendency) before lim! (which applies the limiter to the state).

I'm going to close, and open an issue instead.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants