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

Area index in aux #263

Merged
merged 1 commit into from
Jul 25, 2023
Merged

Area index in aux #263

merged 1 commit into from
Jul 25, 2023

Conversation

kmdeck
Copy link
Member

@kmdeck kmdeck commented Jul 11, 2023

Purpose

  • Removes LAI and h_c from SharedCanopyParameters as those are duplicated in the PlantHydraulicsParameters
  • Allows LAI to vary in time
  • Builds some infrastructure for future parameters that vary in space and time.

To-do

  • future PR: LAI(t) from Ozark data.

Content

  1. LAI and h_c are removed from SharedCanopyParameters. The height of the canopy is now obtained from the last element of "compartment_surfaces" [plant hydraulics compartment surfaces, in the hydraulics model] and LAI is now obtained from the area_index. This removes duplication of parameter definition.
  2. Replace the area_index (the constant named tuple of floats for SAI, RAI, and LAI) in PlantHydraulicsParameters with an area_index "model" (prescribed function for LAI(t), SAI::FT, RAI::FT, or eventually reading from a map).
  3. Adds a field of NamedTuples to the auxiliary state: p.canopy.hydraulics.area_index
  4. adds a "set initial aux state" method for the canopy which sets globally varying parameters (just area_index for now) with their initial values
  5. adds an "update parameter fields" function which can be called in update aux which updates the area index (and eventually any other globally varying fields that change in time).
  6. Adds a utils function for indexing fields of NamedTuples by symbol.

Discussion items

It may be that in the long run, these globally varying fields should be stored in p.canopy directly. we can build that when we need it? distiniguish between prescribed parameters in p and diagnostic params that depend on state? [currently not doing this]

One downside is that for some models (bucket, now canopy too and any model involving the canopy) we need to call "set_initial_aux_state!" and for some we dont. perhaps this should be a standard step in tutorials even if it isnt needed for all models? it could be confusing. [ we decided offline that all models will call this, even if not strictly required]

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 marked this pull request as draft July 11, 2023 23:30
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.

Looks good! I think this is quite in line with what we've discussed. I just had a couple of small suggestions

src/shared_utilities/ntuple_utils.jl Outdated Show resolved Hide resolved
src/standalone/Vegetation/Canopy.jl Outdated Show resolved Hide resolved
src/standalone/Vegetation/Canopy.jl Show resolved Hide resolved
src/standalone/Vegetation/PlantHydraulics.jl Outdated Show resolved Hide resolved
src/standalone/Vegetation/PlantHydraulics.jl Outdated Show resolved Hide resolved
src/standalone/Vegetation/PlantHydraulics.jl Outdated Show resolved Hide resolved
@juliasloan25
Copy link
Member

juliasloan25 commented Jul 20, 2023

Also, for discussion item #2, I agree that all models should call set_initial_aux_state! for consistency, even if they don't need it for their functionality. They can maybe just use the default which calls update_aux (as we discussed earlier today)

Copy link
Member

@AlexisRenchon AlexisRenchon left a comment

Choose a reason for hiding this comment

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

I just had a few food for thoughts comment, but it looks good to me!

@kmdeck
Copy link
Member Author

kmdeck commented Jul 24, 2023

Also, for discussion item #2, I agree that all models should call set_initial_aux_state! for consistency, even if they don't need it for their functionality. They can maybe just use the default which calls update_aux (as we discussed earlier today)

ok! Im going to open an issue for this (to add this to our tests/experiments/docs)

@kmdeck
Copy link
Member Author

kmdeck commented Jul 24, 2023

I just had a few food for thoughts comment, but it looks good to me!

thank you! Ill address your comments.

Do you think you could take on this issue? #275 related to our discussion last week.

@kmdeck kmdeck force-pushed the area_index_in_aux branch from bb451e3 to d4d241e Compare July 24, 2023 23:23
@kmdeck kmdeck changed the title WIP area index in aux Area index in aux Jul 24, 2023
@kmdeck kmdeck marked this pull request as ready for review July 25, 2023 00:07
@kmdeck kmdeck force-pushed the area_index_in_aux branch from be82b95 to 012b514 Compare July 25, 2023 19:49
still wip

Tests pass

gitignore update

Start to work on rev comments

renaming

file rename

lai consistency check

addressing Charlie's comment

revert rename of file

fixed docs

added a test

fixed test

CC 10.44

fixing tests again

docs bug
@kmdeck kmdeck force-pushed the area_index_in_aux branch from 012b514 to 8f2c32f Compare July 25, 2023 20:08
@kmdeck
Copy link
Member Author

kmdeck commented Jul 25, 2023

bors r+

@bors
Copy link
Contributor

bors bot commented Jul 25, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit 4635205 into main Jul 25, 2023
@bors bors bot deleted the area_index_in_aux branch July 25, 2023 20:57
@kmdeck kmdeck mentioned this pull request Jul 25, 2023
15 tasks
mitraA90 pushed a commit that referenced this pull request Dec 22, 2023
263: Area index in aux r=kmdeck a=kmdeck

## Purpose 
- Removes LAI and h_c from SharedCanopyParameters as those are duplicated in the PlantHydraulicsParameters
- Allows LAI to vary in time 
- Builds some infrastructure for future parameters that vary in space and time.


## To-do
- future PR: LAI(t) from Ozark data.

## Content
1. LAI and h_c are removed from SharedCanopyParameters. The height of the canopy is now obtained from the last element of "compartment_surfaces" [plant hydraulics compartment surfaces, in the hydraulics model] and LAI is now obtained from the area_index. This removes duplication of parameter definition.
2. Replace the area_index (the constant named tuple of floats for SAI, RAI, and LAI) in PlantHydraulicsParameters with an area_index "model" (prescribed function for LAI(t), SAI::FT, RAI::FT, or eventually reading from a map).
3. Adds a field of NamedTuples to the auxiliary state: p.canopy.hydraulics.area_index
4. adds a "set initial aux state" method for the canopy which sets globally varying parameters (just area_index for now) with their initial values
5. adds an "update parameter fields" function which can be called in update aux which updates the area index (and eventually any other globally varying fields that change in time).
6. Adds a utils function for indexing fields of NamedTuples by symbol.

## Discussion items
It may be that in the long run, these globally varying fields should be stored in p.canopy directly. we can build that when we need it? distiniguish between prescribed parameters in `p` and diagnostic params that depend on state? [currently not doing this]

One downside is that for some models (bucket, now canopy too and any model involving the canopy) we need to call "set_initial_aux_state!" and for some we dont. perhaps this should be a standard step in tutorials even if it isnt needed for all models? it could be confusing. [ we decided offline that all models will call this, even if not strictly required]

Review checklist

I have:
- followed the codebase contribution guide: https://clima.github.io/ClimateMachine.jl/latest/Contributing/
- followed the style guide: https://clima.github.io/ClimateMachine.jl/latest/DevDocs/CodeStyle/
- followed the documentation policy: https://github.com/CliMA/policies/wiki/Documentation-Policy
- checked that this PR does not duplicate an open PR.

In the Content, I have included 
- relevant unit tests, and integration tests, 
- appropriate docstrings on all functions, structs, and modules, and included relevant documentation.


----
- [x] I have read and checked the items on the review checklist.


Co-authored-by: kmdeck <kdeck@caltech.edu>
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.

4 participants