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

Preallocate thermal state #589

Merged
merged 3 commits into from
May 28, 2024
Merged

Preallocate thermal state #589

merged 3 commits into from
May 28, 2024

Conversation

Sbozzolo
Copy link
Member

@Sbozzolo Sbozzolo commented Apr 23, 2024

@Sbozzolo Sbozzolo force-pushed the gb/preallocate_thermal_state branch 13 times, most recently from 5bedf2e to f5897b9 Compare April 30, 2024 21:59
@Sbozzolo Sbozzolo requested review from kmdeck and juliasloan25 May 1, 2024 00:47
Copy link
Member

@kmdeck kmdeck left a comment

Choose a reason for hiding this comment

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

I think we should discuss where the ts is allocated/stored. In this particular case, I think it could make sense to store it/add it to p in:

  • p.drivers.ts_atmos, and update not with evaluate! like the other driver variables, but instead update with the set_atmos_ts! function you wrote. We already have an update_drivers function which this would be easy to add to. This is consistent also in that it will update only update when update_drivers is called (as a callback), which is when the other drivers are updated, and not every timestep/stage (even though the drivers may not be changing each step).

More generally - if we need to add scratch variables - we can do so pretty easily via our existing auxiliary_vars and boundary_vars functionality. We may already have some, actually. then they would be stored in p.soil.scratch (e.g.) and we can choose which models need them/for which boundary conditions. We could also make a new function like you did and add scratch variables for a model to p.scratch, like we do for driver variables. But that is a little bigger of a job and I dont think it is needed for this PR.

@kmdeck
Copy link
Member

kmdeck commented May 1, 2024

if certain boundary conditions or source terms require scratch, but others dont, we could make a scratch_vars(model) = (scratch_vars(model.boundary_conditions)..., scratch_vars(model.sources)...) function.

@Sbozzolo Sbozzolo force-pushed the gb/preallocate_thermal_state branch from f5897b9 to 7ff391a Compare May 6, 2024 16:56
Copy link
Member

@juliasloan25 juliasloan25 left a comment

Choose a reason for hiding this comment

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

This looks great! I agree with Kat's comment, and it looks like those changes have already been incorporated.

Can we fully remove the construct_atmos_ts function and remaining calls to it now? (e.g. in exp/Standalone/Soil/evaporation.jl, and a couple of the tests)

For GPU compatibility
@Sbozzolo Sbozzolo force-pushed the gb/preallocate_thermal_state branch 9 times, most recently from 0dc9767 to 01f24d2 Compare May 25, 2024 00:23
@Sbozzolo Sbozzolo requested a review from kmdeck May 28, 2024 14:49
@Sbozzolo Sbozzolo force-pushed the gb/preallocate_thermal_state branch from 01f24d2 to 36dc942 Compare May 28, 2024 14:54
@Sbozzolo Sbozzolo force-pushed the gb/preallocate_thermal_state branch from 36dc942 to 87de665 Compare May 28, 2024 15:09
Copy link
Member

@kmdeck kmdeck left a comment

Choose a reason for hiding this comment

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

This looks good to me, thank you! nice improvement to the code.

@Sbozzolo Sbozzolo enabled auto-merge May 28, 2024 21:44
@Sbozzolo Sbozzolo merged commit ae49c2a into main May 28, 2024
9 checks passed
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.

construct_atmos_ts allocates at every step
3 participants