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

initial cache method removal #575

Merged
merged 1 commit into from
May 9, 2024
Merged

Conversation

kmdeck
Copy link
Member

@kmdeck kmdeck commented Mar 29, 2024

Purpose

Removes special methods of set_initial_cache. All models can use the default

To-do

review

Content

  • bucket model method was a duplicate of the default -> easy to remove
  • canopy model was a default except for set_prescribed_canopy_field call. Now we call this in update_aux and can use the default initial cache setting method.
  • LSM model should have been calling update drivers, then update aux for each model, and then update boundary fluxes for each model, since this is what we do each timestep. it was instead caling update drivers, and then set initial cache for each model. Now we use the default, which does what we want.

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.

@kmdeck kmdeck self-assigned this Mar 29, 2024
@kmdeck kmdeck force-pushed the kd/cleanup_set_initial_cache_methods branch from 39c47f2 to 723a10c Compare May 2, 2024 23:22
@kmdeck kmdeck marked this pull request as ready for review May 2, 2024 23:32
@kmdeck kmdeck requested a review from juliasloan25 May 3, 2024 00:07
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 is a great change, thank you Kat!

Comment on lines -141 to -149
These fields are stored in the aux-state and *should not* depend on the prognostic
state `Y` or other diagnostic variables stored in `p`; this allows them
to be updated first, prior to updating the rest of the aux state and prognostic state.

However, there is no
guarantee on the order of operations in terms of when diagnostic auxiliary
variables are updated vs. prescribed field auxiliary variables; any required
order of operations must be enforced by the developer who writes the update_aux
function.
Copy link
Member

Choose a reason for hiding this comment

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

Should we keep this docstring?

Copy link
Member Author

Choose a reason for hiding this comment

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

I cleaned it up a bit, but I think it still holds, so keeping. let me know if you had something in mind for not including it!

@kmdeck kmdeck force-pushed the kd/cleanup_set_initial_cache_methods branch from 9a1404a to eba150e Compare May 8, 2024 22:39
@kmdeck kmdeck merged commit f36042f into main May 9, 2024
9 checks passed
@kmdeck kmdeck deleted the kd/cleanup_set_initial_cache_methods branch May 9, 2024 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants