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

rename fates_fire_crown_depth_frac? #780

Closed
rgknox opened this issue Sep 27, 2021 · 4 comments
Closed

rename fates_fire_crown_depth_frac? #780

rgknox opened this issue Sep 27, 2021 · 4 comments

Comments

@rgknox
Copy link
Contributor

rgknox commented Sep 27, 2021

In PR #738 , we start using the "crown" parameter (fraction of the plant's total height that represents the depth of crown) for hydraulics. Knowing the depth of the crown is important for knowing how high the leaves are above the ground, the height of the leaves impacts their total water potential, which is key for driving fluxes.

Until now this parameter has only been used for fire, and thus, its name prefix "fates_fire" was appropriate to help fire modelers quickly home in on this parameter as relevant. However, now that its important for hydraulics too, I advocate for updating the name and incorporating this into the next batch of updates to the parameter file.

Option 1:

fates_crown_depth_frac

Option 2:

fates_firehydr_crown_depth_frac (or some variant that conveys its used for fire and hydraulics)

Other options/ideas?

FYI @jkshuman

@ckoven
Copy link
Contributor

ckoven commented Sep 27, 2021

Just a note that we already use the parameter for non-fire-related things, in particular for the diagnostic leaf area density distribution, as in: https://github.com/NGEET/fates/blob/master/main/FatesHistoryInterfaceMod.F90#L2221-L2244

Currently that is just a diagnostic, but since we plan to make the leaf area distribution affect things like turbulence, it has some meaning outside of fire and hydraulics already. So I'd advocate for option 1 above.

@jkshuman
Copy link
Contributor

Thanks, @rgknox. I am inclined to remove the tag and go with option 1, but could be convinced about option 2. The fire tag is useful to identify parameters that are exclusive to fire. As things are incorporated into other functions it seems that we should remove the tag. I can imagine the names getting really long otherwise.

@adrifoster thoughts on this in terms of the file updates and best practices?

@adrifoster
Copy link
Contributor

adrifoster commented Sep 27, 2021

I agree with @rgknox , @ckoven and @jkshuman - I think we should remove the "fire_" tag.

Also note that I am also removing the "FIRE_/SPITFIRE_" tag in the output history variable names, but let me know if I shouldn't do this. It was inconsistent on which output variables had them or not, so I figured as long as the variable name/long name made it obvious it was about fire, that was okay.

FATES does not currently have a different fire option (right?) but I suppose if we did we could add in a "SPITFIRE" tag. All the variables will have a "FATES_" prefix, so this will distinguish it from a HLM-specific fire variable.

@rgknox
Copy link
Contributor Author

rgknox commented Sep 27, 2021

Thanks for the quick feedback everyone.
Since this parameter is used in various contexts, it does seem most appropriate to remove the module prefix, ie option 1.

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

No branches or pull requests

4 participants