-
Notifications
You must be signed in to change notification settings - Fork 341
FATES JSON parameter files #3570
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
base: master
Are you sure you want to change the base?
Conversation
|
@rgknox can you add the fates branch you are pointing to here in .gitmodules? I also wanted to know what the python version and module environment requirements will be for the new $FATESDIR/tools/modify_fates_paramfile.py script? If it's just bare python, that will allow us to run these tests in CESM testing which would be a nice thing to have for when this comes in. I just found the FATES PR and it looks like the one requirement is numpy, which doesn't come in by default for bare python. So CESM tests wouldn't be able to run modify_fates_paramfile.py, unless we do something about removing that requirement. Hmmm. |
…also updated shell_commands to use updated file type and scripting
Merge b4b-dev to master PR ESCOMP#3609
|
This is passing fates test suit on derecho, B4B with base |
|
@ekluzek , the python scripts for fates parameter file modifications, including $FATESDIR/tools/modify_fates_paramfile.py, just use the core libraries now. Derecho had no trouble with modify_fates_paramfile.py. Lets see how izumi does.. |
ekluzek
left a comment
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.
Cool. I have some comments and questions from looking at this on my own. But, I'm looking forward to getting the walkthrough to understand in more depth.
| Create the namelist for CLM | ||
| REQUIRED OPTIONS | ||
| -cimeroot "directory" Path to cime directory | ||
| -landroot "directory" Path to the land models parent directory, ie <>/ctsm |
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.
We probably don't need to send landroot in, as we should be able to get it from information already in CLMBuildNamelist.pm
|
|
||
| <fates_paramfile>lnd/clm2/paramdata/fates_params_api.41.0.0_14pft_c250813.nc</fates_paramfile> | ||
|
|
||
| <fates_paramfile>src/fates/parameter_files/fates_params_default.json</fates_paramfile> |
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.
Hmmm. Interesting. Currently we assume that anything with a relative path is under $DIN_LOC_ROOT. But, here we need to point out that it's under LANDROOT. So we should probably add an attribute to say that here.
|
|
||
| <entry id="fates_paramfile" type="char*512" category="datasets" | ||
| input_pathname="abs" group="clm_inparm" valid_values="" > | ||
| input_pathname="landroot" group="clm_inparm" valid_values="" > |
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.
Ahhh, here is where you are pointing out that it's under landroot.
|
|
||
| ! It is FATES' job to open and close the file | ||
| ! This step is simply to provide a unit number to use | ||
| fates_paramfile_unit = shr_file_getUnit() |
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.
shr_file_getUnit is actually deprecated now that Fortran has built-in file unit management. When you use open to open a file you can use newunit= to get a free unit. So it simplifies how this is done.
| @@ -1,235 +0,0 @@ | |||
| module CLMFatesParamInterfaceMod | |||
| ! NOTE(bja, 2017-01) this code can not go into the main clm-fates | |||
| ! interface module because of circular dependancies with pftvarcon. | |||
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.
Wow, you were able to remove this entire file?! That's amazing.
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.
So basically this file that's HLM specific gets replaced by FatesReportParameters which is in FATES so it's reused by all of the HLM's, which is a great direction to go.
|
|
||
| $FATESDIR/tools/modify_fates_paramfile.py --overwrite --fin $FATESPARAMFILE --param fates_cnp_prescribed_puptake --values 1.0 --indices all | ||
|
|
||
| echo "fates_paramfile = '$FATESPARAMFILE'" >> $CASEDIR/user_nl_clm |
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.
I'm a bit concerned about this bit about adding it to the end of the user_nl_clm. A small thing is that each time shell_commands is run it'll append it to the end again. It still works -- but looks a bit strange.
But, putting it here could conflict with what the user adds to a test case. Since, it is a test case that seems unlikely -- but I wonder about unintended consequences. This is a part that can be removed when we can move forward on the plan to move fates parameter file handling into buildnml. So it's likely fine to do as a first step.
| </phase> | ||
| </test> | ||
|
|
||
| <test name="ERI_D_Ld9.f45_f45_mg37.I2000Clm60FatesSpCruRsGs.derecho_gnu.clm-FatesColdSatPhenCamLndTuningMode"> |
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 fixes these two tests? Or is this something that was fixed with the nextsw change, that just didn't get these removed from ExpectedFails?
Description of changes
These changes accommodate a JSON format to the FATES parameter file. This set of changes is synchronized with FATES 1493.
This is a work in progress.
Specific notes
Contributors other than yourself, if any:
CTSM Issues Fixed (include github issue #):
Work on #2126
Are answers expected to change (and if so in what way)? No
Any User Interface Changes (namelist or namelist defaults changes)? Change FATES parameter file to JSON format
Does this create a need to change or add documentation? Did you do so? No
Testing performed, if any: individual testing, will run aux_clm and fates on Derecho and Izumi