-
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
Make certain history fields descriptive that have been labeled by number since #1340 #1413
Conversation
My work this far in this PR has resolved the problem that I introduced in #1340 for certain litter fields. Next I will update the handling of other history fields that still get labeled by number instead of by descriptive string. |
fieldname = 'DWT_FROOTC_TO_LITR_'//trim(decomp_cascade_con%decomp_pool_name_short(k))//'_C' | ||
longname = 'fine root to '//trim(decomp_cascade_con%decomp_pool_name_long(k))//' litter due to landcover change' | ||
fieldname = 'DWT_FROOTC_TO_'//trim(decomp_cascade_con%decomp_pool_name_history(k))//'_C' | ||
longname = 'fine root to '//trim(decomp_cascade_con%decomp_pool_name_long(k))//' due to landcover change' |
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.
Here and elsewhere I have now removed the hardwired strings "LITR_" and "litter" and placed them in decomp_pool_name_history
and decomp_pool_name_long
.
decomp_cascade_con%decomp_pool_name_short(i_met_lit) = 'MET' | ||
decomp_cascade_con%decomp_pool_name_history(i_met_lit) = 'MET_LIT' | ||
decomp_cascade_con%decomp_pool_name_long(i_met_lit) = 'metabolic litter' | ||
decomp_cascade_con%decomp_pool_name_short(i_met_lit) = 'L1' |
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.
Here and elsewhere I updated decomp_pool_name_history
and decomp_pool_name_long
with descriptive names rather than numbers. However:
- I kept
decomp_pool_name_restart
unchanged because otherwise I lost the bfb answers with baseline. - I returned
decomp_pool_name_short
to the original non-descriptive names to maintain the option of truly short names. These came in handy for certain history fields that otherwise triggered an error by exceeding some length.
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.
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 realized this because 6 cheyenne tests failed with the error
LITR1C_TO_SOIL1C in fincl( 22 ) for history tape 1 not found
Otherwise both test-suites were OK.
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.
Yes, this sounds.
decomp_cascade_con%decomp_pool_name_history(i_pro_som) = 'SOIL1' | ||
decomp_cascade_con%decomp_pool_name_long(i_pro_som) = 'soil 1' | ||
decomp_cascade_con%decomp_pool_name_history(i_pro_som) = 'PAS_SOM' | ||
decomp_cascade_con%decomp_pool_name_long(i_pro_som) = 'passive soil organic matter' |
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.
@wwieder let's make sure that I labeled the three SOMs correctly. In the first refactor I had chosen other names for the corresponding indices:
- i_pro_som for "protected"
- i_rec_som for "recalcitrant"
- i_avl_som for "available"
If I have now correctly mapped "protected" to "passive", "recalcitrant" to "slow", and "available" to "active", then I think I should also rename the indices for consistency to i_pas_som, i_slo_som, and i_act_som. Let me know if you disagree.
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 is likely fine, but we should add carbon and nitrogen to both name_history
and name_long
(as is done with the BGC / CENTURY pools).
To your other question, let's go ahead and call the pools active, slow and passive (this is more consistent with the CENTURY language). And, yes, your mapping is correct.
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 is likely fine, but we should add carbon and nitrogen to both
name_history
andname_long
(as is done with the BGC / CENTURY pools).
The C/carbon and N/nitrogen labels get added to the history fields when the variables are written to history. This way the section of the code that we are looking at here can exist once for both C and N.
@@ -340,7 +340,7 @@ subroutine InitHistory(this, bounds) | |||
data1dptr => this%decomp_cascade_sminn_flux_col(:,l) | |||
if ( decomp_cascade_con%cascade_receiver_pool(l) /= 0 ) then | |||
fieldname = 'SMINN_TO_'//& | |||
trim(decomp_cascade_con%decomp_pool_name_history(decomp_cascade_con%cascade_receiver_pool(l)))//'N_'//& | |||
trim(decomp_cascade_con%decomp_pool_name_short(decomp_cascade_con%cascade_receiver_pool(l)))//'N_'//& |
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.
Here and in one other place I replaced decomp_pool_name_history
with decomp_pool_name_short
because I got an error as a result of a fieldname that was too long.
The longname still contains all the necessary information for users to decipher which variable they are looking at.
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.
Yes, this makes sense.
hist_fexcl1 += 'SOIL1C_vr', 'SOIL1N_vr', 'SOIL2C_vr', 'SOIL2N_vr', 'SOIL3C_vr', 'SOIL3N_vr', 'SOILC_vr','SOILN_vr', 'CWDC_vr', 'LITR1C_vr', 'LITR2C_vr', 'LITR3C_vr', 'LITR1N_vr', 'LITR2N_vr', 'LITR3N_vr', 'CWDN_vr', 'SMIN_NO3_vr', 'CONC_O2_UNSAT', 'CONC_O2_SAT','SMIN_NH4_vr','SMINN_vr' | ||
hist_fincl1 += 'LEAFC_TO_LITTER', 'FROOTC_TO_LITTER','LITR1C_TO_SOIL1C','LITR1N_TO_SOIL1N','LITR2C_TO_SOIL1C', 'LITR2N_TO_SOIL1N','LITR3C_TO_SOIL2C','LITR3N_TO_SOIL2N','DWT_WOOD_PRODUCTC_GAIN_PATCH' | ||
hist_fexcl1 += 'PAS_SOMC_vr', 'PAS_SOMN_vr', 'SLO_SOMC_vr', 'SLO_SOMN_vr', 'ACT_SOMC_vr', 'ACT_SOMN_vr', 'SOILC_vr','SOILN_vr', 'CWDC_vr', 'MET_LITC_vr', 'CEL_LITC_vr', 'LIG_LITC_vr', 'MET_LITN_vr', 'CEL_LITN_vr', 'LIG_LITN_vr', 'CWDN_vr', 'SMIN_NO3_vr', 'CONC_O2_UNSAT', 'CONC_O2_SAT','SMIN_NH4_vr','SMINN_vr' | ||
hist_fincl1 += 'LEAFC_TO_LITTER', 'FROOTC_TO_LITTER','MET_LITC_TO_PAS_SOMC','MET_LITN_TO_PAS_SOMN','CEL_LITC_TO_PAS_SOMC', 'CEL_LITN_TO_PAS_SOMN','LIG_LITC_TO_SLO_SOMC','LIG_LITN_TO_SLO_SOMN','DWT_WOOD_PRODUCTC_GAIN_PATCH' |
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.
Cheyenne test-suite now OK (with the changes to this file).
Izumi test-suite was already OK before.
Now only doc/source/users_guide/setting-up-and-running-a-case/master_list_file.rst
(i.e. the documentation) is inconsistent.
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 with other files, SOIL1C needs to be ACT_SOM
New stream functionality when using NUOPC or LILAC This tag creates new stream functionality when using the NUOPC and LILAC caps, and maintains backwards compatibility with the older CPL7 stream functionality when using the MCT cap. - New stream code has been placed in src/cpl/share_esmf (so that they can also be leveraged by LILAC) - Older stream code for mct is now in src/cpl/mct - LILAC interface has been changed to leverage the new stream code as well Where possible share code was used. In particular, this holds for SatellitePhenologyMod.F90. Resolved Conflicts: doc/ChangeLog doc/ChangeSum
Params file sets the respiration fractions for CWD to zero: rf_cwdl2_bgc = 0 rf_cwdl3_bgc = 0 The model has no code to handle rf_cwd* > 0. So... - I have introduced cwdhr_col to track this HR the same way that we track lithr_col for litter. - I have removed if-statements that prevented CWD HR pools and decomposition from writing to history. - I have introduced a new test to the cheyenne test-suite that makes active seven history fields pertaining to CWD HR and points to a params file with rf_cwd* > 0. I have confirmed that the new test returns the new active variables as greater than 0 when pointing to the new params file and equal to 0 when pointing to the default params file. NB: This branch and PR ESCOMP#1413 started from ESCOMP#1400 which already included almost all the code that would have otherwise come in with this merge. Resolved Conflicts: doc/ChangeLog doc/ChangeSum
I have now rebuilt the official documentation. I will wait for this PR to be merged before pushing the new documentation. |
Now that I updated to dev045, |
@slevisconsulting what's the status on this? |
Ready for final review and line-up for merge. As soon as I hear that this PR is next in line, I can rerun the test-suites. |
@ekluzek can you coordinate with @slevisconsulting on timing for this PR? |
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.
Throughout S1 should correspond to active SOM, and S3 should be passive. Sorry for the confusion @slevisconsulting
is_metabolic(i_avl_som) = .false. | ||
is_cellulose(i_avl_som) = .false. | ||
is_lignin(i_avl_som) = .false. | ||
i_pas_som = i_lig_lit + 1 |
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 should be active SOM (S1)? Passive should be the last pool (S3).
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.
Thank you for catching @wwieder! I will correct this later today.
is_cellulose(i_slo_som) = .false. | ||
is_lignin(i_slo_som) = .false. | ||
|
||
i_act_som = i_slo_som + 1 |
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 here these fields should be the passive SOM(S3), NOT active
@@ -480,10 +480,10 @@ subroutine init_decompcascade_bgc(bounds, soilbiogeochem_state_inst, soilstate_i | |||
spinup_factor(i_cwd) = max(1._r8, (speedup_fac * params_inst%tau_cwd_bgc / 2._r8 )) | |||
end if | |||
!som1 | |||
spinup_factor(i_pro_som) = 1._r8 | |||
spinup_factor(i_pas_som) = 1._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.
again, this should be active, slow passive for S1, S2, S3, respectively
@@ -495,56 +495,56 @@ subroutine init_decompcascade_bgc(bounds, soilbiogeochem_state_inst, soilstate_i | |||
decomp_cascade_con%cascade_step_name(i_l1s1) = 'L1S1' | |||
rf_decomp_cascade(bounds%begc:bounds%endc,1:nlevdecomp,i_l1s1) = rf_l1s1 | |||
cascade_donor_pool(i_l1s1) = i_met_lit | |||
cascade_receiver_pool(i_l1s1) = i_pro_som | |||
cascade_receiver_pool(i_l1s1) = i_pas_som |
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.
again, active here and the in lines below
pathfrac_decomp_cascade(bounds%begc:bounds%endc,1:nlevdecomp,i_l3s2) = 1.0_r8 | ||
|
||
i_s1s2 = 4 | ||
decomp_cascade_con%cascade_step_name(i_s1s2) = 'S1S2' | ||
rf_decomp_cascade(bounds%begc:bounds%endc,1:nlevdecomp,i_s1s2) = rf_s1s2(bounds%begc:bounds%endc,1:nlevdecomp) | ||
cascade_donor_pool(i_s1s2) = i_pro_som | ||
cascade_receiver_pool(i_s1s2) = i_rec_som | ||
cascade_donor_pool(i_s1s2) = i_pas_som |
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.
again S1 should be active
cascade_donor_pool(i_s2s3) = i_rec_som | ||
cascade_receiver_pool(i_s2s3) = i_avl_som | ||
cascade_donor_pool(i_s2s3) = i_slo_som | ||
cascade_receiver_pool(i_s2s3) = i_act_som |
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 passive here for S3
pathfrac_decomp_cascade(bounds%begc:bounds%endc,1:nlevdecomp,i_s2s3) = f_s2s3 | ||
|
||
i_s3s1 = 8 | ||
decomp_cascade_con%cascade_step_name(i_s3s1) = 'S3S1' | ||
rf_decomp_cascade(bounds%begc:bounds%endc,1:nlevdecomp,i_s3s1) = rf_s3s1 | ||
cascade_donor_pool(i_s3s1) = i_avl_som | ||
cascade_receiver_pool(i_s3s1) = i_pro_som | ||
cascade_donor_pool(i_s3s1) = i_act_som |
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.
switch here
depth_scalar(c,j) * o_scalar(c,j) * spinup_geogterm_s2(c) | ||
decomp_k(c,j,i_avl_som) = k_s3 * t_scalar(c,j) * w_scalar(c,j) * & | ||
decomp_k(c,j,i_act_som) = k_s3 * t_scalar(c,j) * w_scalar(c,j) * & |
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.
again, switch active and passive here (S1 and S3, respectively)
hist_fexcl1 += 'SOIL1C_vr', 'SOIL1N_vr', 'SOIL2C_vr', 'SOIL2N_vr', 'SOIL3C_vr', 'SOIL3N_vr', 'SOILC_vr','SOILN_vr', 'CWDC_vr', 'LITR1C_vr', 'LITR2C_vr', 'LITR3C_vr', 'LITR1N_vr', 'LITR2N_vr', 'LITR3N_vr', 'CWDN_vr', 'SMIN_NO3_vr', 'CONC_O2_UNSAT', 'CONC_O2_SAT','SMIN_NH4_vr','SMINN_vr' | ||
hist_fincl1 += 'LEAFC_TO_LITTER', 'FROOTC_TO_LITTER','LITR1C_TO_SOIL1C','LITR1N_TO_SOIL1N','LITR2C_TO_SOIL1C', 'LITR2N_TO_SOIL1N','LITR3C_TO_SOIL2C','LITR3N_TO_SOIL2N','DWT_WOOD_PRODUCTC_GAIN_PATCH' | ||
hist_fexcl1 += 'PAS_SOMC_vr', 'PAS_SOMN_vr', 'SLO_SOMC_vr', 'SLO_SOMN_vr', 'ACT_SOMC_vr', 'ACT_SOMN_vr', 'SOILC_vr','SOILN_vr', 'CWDC_vr', 'MET_LITC_vr', 'CEL_LITC_vr', 'LIG_LITC_vr', 'MET_LITN_vr', 'CEL_LITN_vr', 'LIG_LITN_vr', 'CWDN_vr', 'SMIN_NO3_vr', 'CONC_O2_UNSAT', 'CONC_O2_SAT','SMIN_NH4_vr','SMINN_vr' | ||
hist_fincl1 += 'LEAFC_TO_LITTER', 'FROOTC_TO_LITTER','MET_LITC_TO_PAS_SOMC','MET_LITN_TO_PAS_SOMN','CEL_LITC_TO_PAS_SOMC', 'CEL_LITN_TO_PAS_SOMN','LIG_LITC_TO_SLO_SOMC','LIG_LITN_TO_SLO_SOMN','DWT_WOOD_PRODUCTC_GAIN_PATCH' |
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 with other files, SOIL1C needs to be ACT_SOM
I had assigned passive SOM to soil1 and active SOM to soil3 and should have assigned them the other way around. The correction results in a modified master_list_file.rst. I have rebuilt the official documentation and will push when this PR gets merged.
Again, thank you for catching this mistake @wwieder ! |
@slevisconsulting this should be updated to ctsm5.1.dev047 and run the full testing on it, and then bring it in as ctsm5.1.dev048. |
Start bounds at 1; remove references to MCT Users: take note of (1), and in particular the caveat for users noted below (in the "Notes of particular relevance for users"). (1) All global index arrays on a given processor now have a starting index of 1 - bounds_proc for each subgrid level has a starting index of 1 for each level - bounds_clump for each subgrid level has a starting index of 1 for just the first clump on the processor - but all the other clumps on the processor do not start at 1 - but rather are offset with the number of gridcells, columns, ...etc on the preceeding clumps (2) There are no longer any references to any mct data structures other than in the mct cap - All references to gsmap have been removed from decompMod.F90 and replaced with new global index arrays for the various subgrid levels - decompInitMod has been refactored to calculated these global index arrays using pure MPI rather than mct - ncdio_pio_F90.in has been refactored to use the new global index arrays rather than the gsmap data structures - the data struture ldecomp is no longer needed - the module spmdGathScatMod.F90 is no longer needed and has been removed Resolved Conflicts: doc/ChangeLog doc/ChangeSum
Cheyenne test-suite OK Izumi test-suite OK |
Description of changes
I use existing infrastructure to add descriptive strings to certain history fields that I had labeled by number in #1340
Specific notes
Contributors other than yourself, if any:
@wwieder @ekluzek
CTSM Issues Fixed (include github issue #):
Fixes #1392
Are answers expected to change (and if so in what way)?
No
Any User Interface Changes (namelist or namelist defaults changes)?
No
Testing performed, if any:
./create_test ERP_D_Ld3_P36x2.f10_f10_mg37.I2000Clm50BgcCru.cheyenne_intel.clm-default -c /glade/p/cgd/tss/ctsm_baselines/ctsm5.1.dev043
where I made the C12 and the N variables active temporarily.
The test passed with the only diff. from baseline as follows:
as expected. Previously these fields would have been numbered instead of labeled CEL, LIG, MET.