-
Notifications
You must be signed in to change notification settings - Fork 312
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
PPE parameter work -Addresses issues 2830 and 2831 #2845
base: b4b-dev
Are you sure you want to change the base?
Conversation
Jmaxb1 (now called jmaxb1 and pft-dependent) upplim_destruct_metamorph maximum_leaf_wetted_fraction interception_fraction
…monthly_matrixcn_soilCN30
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.
@olyson thank you for your work in this PR. My comments are mostly minor. The one about whether to read params in pftconMod or locally is a bit bigger, and I'm not certain whether it's relevant and worth considering.
<paramfile phys="clm6_0" >/glade/campaign/cgd/tss/people/oleson/modify_param/ctsm60_params.c241017.nc</paramfile> | ||
<paramfile phys="clm5_1" >/glade/campaign/cgd/tss/people/oleson/modify_param/ctsm51_params.c241017.nc</paramfile> | ||
<paramfile phys="clm5_0" >/glade/campaign/cgd/tss/people/oleson/modify_param/clm50_params.c241017.nc</paramfile> | ||
<paramfile phys="clm4_5" >/glade/campaign/cgd/tss/people/oleson/modify_param/clm45_params.c241017.nc</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.
@olyson at this point I will
- copy the new files to .../lnd/clm2/paramdata and
- ./rimport them
...and you
- can change the paths here.
@@ -875,6 +875,15 @@ | |||
<option name="comment" >Run matrix solution fast spinup for restart beyond a year (to trigger next year logic) turning off extra options to make faster (so without crop or isotopes). This test would've caught a previous bug.</option> | |||
</options> | |||
</test> | |||
<test name="SMS_Lm1" grid="f10_f10_mg37" compset="I1850Clm60Bgc" testmods="clm/clm60_monthly_matrixcn_soilCN30"> |
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.
@olyson thank you for adding this back.
@@ -1,2 +1,2 @@ | |||
paramfile = '$DIN_LOC_ROOT/lnd/clm2/paramdata/ctsm51_ciso_cwd_hr_params.c240814.nc' | |||
paramfile = '/glade/campaign/cgd/tss/people/oleson/modify_param/ctsm51_ciso_cwd_hr_params.c241017.nc' |
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.
- @slevis-lmwg will copy file and ./rimport it
- @olyson will update the path
@@ -1,2 +1,2 @@ | |||
use_soil_matrixcn = .true. | |||
paramfile = '$DIN_LOC_ROOT/lnd/clm2/paramdata/ctsm60_params_cn30.c240822.nc' | |||
paramfile = '/glade/campaign/cgd/tss/people/oleson/modify_param/ctsm60_params_cn30.c241017.nc' |
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.
- @slevis-lmwg will copy file and ./rimport it
- @olyson will update the path
src/biogeochem/CNGapMortalityMod.F90
Outdated
@@ -216,12 +209,12 @@ subroutine CNGapMortality (bounds, num_soilp, filter_soilp, & | |||
am = min(1._r8, am + heatstress(p)) | |||
else ! lpj didn't set this for grasses; cn does | |||
! set the mortality rate based on annual rate | |||
am = params_inst%am | |||
am = r_mort(ivt(p)) |
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 saw that you removed "am" from line 37 above. That makes me wonder whether line 212 -- and the "am = min(..." three lines above -- are still used somewhere?
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.
That is a good point. The variable am is within a use_cndv if block which is now deprecated. I don't think we have any CNDV tests anymore, but if we did I think we wouldn't get the same answers as before with my changes here because "m" would not longer be multiplied by "am". In theory we could remove that whole section of code. But if we don't want to remove it for some reason. I guess I could change the code to this, so that if use_cndv is invoked, then am will be used, as it was before. But we don't have any CNDV testing to check this.
do fp = 1,num_soilp
p = filter_soilp(fp)
if (use_cndv) then
! Stress mortality from lpj's subr Mortality.
if (woody(ivt(p)) == 1._r8) then
if (ivt(p) == 8) then
mort_max = 0.03_r8 ! BDT boreal
else
mort_max = 0.01_r8 ! original value for all patches
end if
! heatstress and greffic calculated in Establishment once/yr
! Mortality rate inversely related to growth efficiency
! (Prentice et al 1993)
am = mort_max / (1._r8 + k_mort * greffic(p))
! Mortality rate inversely related to growth efficiency
! (Prentice et al 1993)
am = mort_max / (1._r8 + k_mort * greffic(p))
am = min(1._r8, am + heatstress(p))
else ! lpj didn't set this for grasses; cn does
! set the mortality rate based on annual rate
am = r_mort(ivt(p))
end if
else
am = r_mort(ivt(p))
end if
m = am/(get_average_days_per_year() * secspday)
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.
As far as I know, cndv has not worked (or not worked correctly, at least) for quite some time. So I would be fine with removing cndv code. I don't know if @ekluzek would agree.
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.
Thanks @slevis-lmwg . I'm thinking now that maybe it wouldn't be a good idea to remove the cndv code piecemeal. I think for now I'll make the code mod I suggested above.
src/main/pftconMod.F90
Outdated
real(r8), allocatable :: jmaxb0(:) ! Baseline proportion of nitrogen allocated for electron transport (J) | ||
real(r8), allocatable :: jmaxb1(:) ! Coefficient determining the response of electron transport rate to light availability (-) | ||
real(r8), allocatable :: wc2wjb0(:) ! The baseline ratio of rubisco limited rate vs light limited photosynthetic rate (Wc:Wj) (-) | ||
real(r8), allocatable :: r_mort(:) ! Mortality rate (1/year) |
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.
@olyson I understand that the code works as written. I will ask this anyway in case it's relevant and worth considering:
As a general rule we try to place here (pftconMod) the params that appear in multiple places in the code. On the other hand, we try to place params that appear in a single module in the module where they appear.
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.
Right, I started down the path of placing the parameters within LunaMod, but realized we don't have the infrastructure in there to do that (e.g., as is done in PhotosynthesisMod.F90 with the photo_params_type and subroutines allocParams, cleanParams, etc.). Much easier to put them in pftconMod.F90 instead of putting all of that infrastructure in, and I wasn't sure what the real value of all that was anyway. But if there is consensus to do it that way, I can do it.
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 am also often motivated by efficiency in code development, so I will let @ekluzek have final say about this.
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.
Thinking about it some more, now that my head is in parameter space, I think I'll just put the work in now to have the parameters read in in LunaMod.F90.
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.
And CNGapMortalityMod.F90 (for r_mort).
…to CNGapMortality
I think this is ready for re-review. Thanks for the input @slevis-lmwg . |
I see now that @slevis-lmwg has rimported the parameter files so I have changed the paths accordingly. Looks like you planned 3997 tests but ran 3996.Does this have anything to do with the jmax test I removed? Is there something else I should remove? |
@slevis-lmwg , it looks like permissions need to be changed on those parameter files, thanks. |
Description of changes
b4b moves of namelist variables to parameter file for PPE per issue #2830 and make some parameter variables pft-dimensioned per #2831
Note that baseflow_scalar and pot_hmn_ign_counts_alpha were not moved from namelist to parameter file due to reasons detailed in #2830
Specific notes
Contributors other than yourself, if any: @slevis-lmwg helped troubleshoot testing issues
CTSM Issues Fixed (include github issue #): #2830 , #2831
Are answers expected to change (and if so in what way)? No
Any User Interface Changes (namelist or namelist defaults changes)? Yes
Does this create a need to change or add documentation? Did you do so? No
Testing performed, if any:
Compare to ctsm5.3.009
Derecho aux_clm: PASS except for (not sure why the baseline files are missing, the last one is ok since it is a new test):
Izumi aux_clm: PASS except for (ok though):
nohup ./build-namelist_test.pl PASS
./run_ctsm_py_tests -u PASS (OK)
./run_ctsm_py_tests -s PASS (OK)