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.

Sign up for GitHub

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

change the order of passing the "use_fates_luh" flag #2898

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

change the order of passing the "use_fates_luh" flag #2898

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b441507
moved the passing of the use_luh2 flag to fates earlier in the sequen…
rgknox Dec 4, 2024
4bff2f2
updated naming convention of local variables
rgknox Dec 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions src/utils/clmfates_interfaceMod.F90
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,6 +305,7 @@ subroutine CLMFatesGlobals1(surf_numpft,surf_numcft,maxsoil_patches)
integer :: pass_nocomp
integer :: pass_sp
integer :: pass_masterproc
integer :: pass_luh
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could the existing pass_use_luh not be used for consistency in terms of naming (i.e., with pass_use_potentialveg)? If not, pass_use_luh seems to not be used anymore, so it should be deleted.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, i can change it. There are other flags in ClmFatesGlobals1 that dropped the "use" in the local variable, I can add those back in too.

Copy link
Collaborator

Choose a reason for hiding this comment

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

One concern I have is with the use of the term potentialveg in FATES code. FATES is using it with one specific meaning, but in CTSM we use PotentialVeg to mean a surface dataset that doesn't have anthropogenic changes. There are similarities, but I'm concerned this might create confusion in CTSM.

As such I'd prefer pass_use_luh for now...

Should we maybe talk about this tomorrow to make sure the use of potentialveg in FATES isn't conflicting with how CTSM uses it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@rgknox I would throw in for adding the "use" back in, because it's a convention we use in CTSM, so just looking at the variable name I know it's a logical variable. And I like that.

But, if we removed the "use" previously -- maybe there was a reason for it that we should look up?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I previously removed the "use" just to be less verbose, nothing more. I'm fine with adding it back in.
@ekluzek, the pass_use_potentialveg is a separate flag from pass_use_luh, I think @samsrabin was pointing it out as an example of convention.
I'm happy to expand this PR though if you would like to update the name of that flag to something less "overlappy" with native ctsm. @ckoven, could you weigh in here on potential alternative names for use_fates_potentialveg.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@glemieux suggested something like use_landuse_spinup.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi All -- My preference is to use "Potential Veg" or similar, rather than "Spinup". The reason is that potential vegetation is a more accurate description of what the relevant code does. Spinup is more of a workflow concept; a user might use potential vegetation apart from spinup, and might do spinup without using potential vegetation mode. It also might make it sound to a user like potential veg has something to do with AD spinup, which it doesn't. In terms of the conceptual overlap of how the phrase is used in non-FATES CTSM, I think that it makes sense that both vegetation models might use the same vocabulary for something that is conceptually similar? I guess I don't see why that is a problem. It seems sort of analogous to how both models have stomatal conductance switches that share names. But I guess another possibility would be to call it something like "All Primary Land Mode"? But I feel like that is more verbose.

logical :: verbose_output
type(fates_param_reader_ctsm_impl) :: var_reader

Expand Down Expand Up @@ -348,6 +349,14 @@ subroutine CLMFatesGlobals1(surf_numpft,surf_numcft,maxsoil_patches)
end if
call set_fates_ctrlparms('masterproc',ival=pass_masterproc)

! FATES landuse modes
if(use_fates_luh) then
pass_luh = 1
else
pass_luh = 0
end if
call set_fates_ctrlparms('use_luh2',ival=pass_luh)

end if


Expand Down Expand Up @@ -540,16 +549,12 @@ subroutine CLMFatesGlobals2()

! FATES landuse modes
if(use_fates_luh) then
pass_use_luh = 1
pass_num_luh_states = num_landuse_state_vars
pass_num_luh_transitions = num_landuse_transition_vars
else
pass_use_luh = 0
pass_num_luh_states = 0
pass_num_luh_transitions = 0
end if

call set_fates_ctrlparms('use_luh2',ival=pass_use_luh)
call set_fates_ctrlparms('num_luh2_states',ival=pass_num_luh_states)
call set_fates_ctrlparms('num_luh2_transitions',ival=pass_num_luh_transitions)

Expand Down
Loading