-
Notifications
You must be signed in to change notification settings - Fork 318
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
Surface roughness modifications #1596
Conversation
…ime output (the latter is still in development). This version of the code is running stable.
I know the date for your paper isn't completely known right now (hence the XXXX in the namelist name). But, it seems like we'll want a real year when this comes into main-dev of CTSM. So I propose we go with Meir2022? I think in the context of namelist options for CTSM being off a bit in the namelist name is acceptable. Especially if the exact reference is pointed to in the code. And we should put the reference in appropriate places where it's refereed to. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RonnyMeier thanks so much for your work on this. This looks really good to me overall. I like the namelist control you added in, as well as the use of case statements for choices. You did a great job with the build-namelist work as well.
One thing that would help with this is that this currently includes changes outside of surface roughness. It also includes running PFT's on their own column, which is in a different PR to come in (#1249), and local time history averaging which already came in PR #1374. It would help to see this without both of those.
It'll likely help prevent conflicts in merging forward, if the local time history changes are removed from here. The update to a newer version will automatically bring it back in anyway.
I also think it's good to have the PFT on a column as a separate PR as well. I remember discussing that one and I don't remember what our conclusion was on including it. I'll make a point of having the software group discuss it more again next week. So removing that would allow us to bring it in separately.
I have some detailed questions that I've put in the code as well. Mainly there is some commented out code that looks like it's for a different option. If you had time to develop it into a third option I think that would be the best path forward. But, because we only have a week, I think the best would be to just remove the commented out code.
I also noticed repeated hardcoded constants. Some of these existed before your work in the code, so aren't really your fault, but still would be good to change into parameters. If they are used only in one file, making them local parameters would be fine. If they are used in more than one file adding them to clm_varcon might be better. If they are something that someone might want to tune, they could be put on the paramsfile. Just elevating them to a local parameter helps though in any case.
Separately I also suggest that MeirXXXX become Meir2022, but we can discuss that. And feel free to discuss any of this, there's always room for talking this over.
But, good work here. Thanks for your contribution. Best of luck on your future endeavors, outside of academia.
|
||
case ('MeierXXXX') | ||
lt = max(0.00001_r8,elai(p)+esai(p)) | ||
displa(p) = htop(p) * (1._r8 - (1._r8 - exp(-(7.5_r8 * lt)**0.5_r8)) / (7.5_r8*lt)**0.5_r8) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
7.5_r8 should be made into a parameter that helps describe what it is. Possibly the same for other variables here as well: 0.00001_r8, some of the 2's and 4's?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still should happen.
Thanks @ekluzek for reviewing the code. I don't have any strong opinions regarding the tag. Meier2022 should most likely be appropriate. We can also go for MeierDavin2022 or GMD2022. Not sure if CESM has any guidlines on how to choose such tags. The commented-out code sections marked with "! --> Use this for CLM-YY" were used for the sensitivity tests that are presented in the appendix of the GMD study. Those can all be removed (sorry I forgot to do this). What could be considered to be included as an additional option is the formulation of Owen and Thomson (1963) to compute z0hg and z0qg. Also, I realized that we actually opened an issue for this development a couple of months ago: New parameterization of vegetation surface roughness for forests #1316 |
…lowing meeting at January 12 2022.
…ghness as htop isn't set yet. Also use a different threshold for snomelt_accum suggested by Ronny Meier
We talked about this a bit this morning. One aspect that we should keep in mind about this is how this interacts with FATES. Our testing ensures we won't break FATES with it. Two questions are can part of it be turned with FATES, and secondly can/should FATES be updated to bring these kind of changes to the FATES side of things? Before we do that I'd like to make a little more progress so the code is a little more clearer for a FATES scientist to evaluate. Specifically, the next steps for this are:
|
I agree @dlawrenncar this quickly gets into questions about preferred configurations for the model moving forward. Looking more carefully at the transition to CRUJRA-TRENDY seems like a good idea |
Per the CLM business meeting on 06/09/22, we decided to use zetamaxstable=2 for all surfaces, and further investigate setting the forcing height to 10m along with implementing CRUJRA as our default dataset but as a separate issue. Fix the subscript overflow in DEBUG mode |
@dmleung made a summary of the differences between the 2005 and the 2012 Prigent datasets that I'm uploading here: |
In @dleung work #1712 has the Prigent data as drag partition factor which relates to the Meir data as follows:
We should compare the two datasets to make sure they are reasonably similar. We think they will be different because the @dmlenung data is the minimum. |
The thinking now is that for the 2D data here, we'll use the data from Leung 2022 (see #1712) and convert it back to what's needed here. So we won't use the Meier data that was processed partially because it's only over non-vegetated areas. We also think the choice of Leung to use the minimum makes sense physically. |
…t work will be brought in with seperate work
Conflicts: cime_config/testdefs/testmods_dirs/clm/Meier2022_surf_rough/user_nl_clm src/biogeophys/FrictionVelocityMod.F90
TODOs
|
@RonnyMeier @ekluzek @RonnyMeier can you make me a collaborator on your fork of CTSM? On your fork go to the "Settings" tab and then "Collaborators" and you should see a way to add a collaborator where you can invite @slevis-lmwg and give me permission to work on your fork. |
@slevis-lmwg we haven't been able to get ahold of Ronny, so we can't push to his forks. What you'll have to do is to branch from his branch and create a branch on your own fork that will include this one. We'll want to keep both PR's open for at least a little while to make sure everything is transferred over. And this PR will autoclose when the new branch brings it to main-dev. so it's actually OK to leave it open until then. |
Superseded by #2045 |
Surface roughness modifications (PR cont'd from #1596 )
This branch contains the surface roughness (z0) modifications published in: https://doi.org/10.5194/gmd-15-2365-2022
When changing the namelist input z0param_method from ZengWang2007 (default) to Meier2022 the following modifications are activated:
Note that the study in GMD also proposes new globally constant values for the z0m of bare soil, snow, and ice. To "activate" those the parameter file needs to be changed at the moment. The original and modified parameter files and surfdata files will be shared by ftp.
Open issues/questions (already discussed with @ekluzek, @dlawrenncar, and @olyson:
Issues Resolved: #1316