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

Added code to put the PCT_NAT_PFT_MAX and PCT_CFT_MAX on the landuse.… #331

Merged
merged 6 commits into from
Jul 6, 2018

Conversation

lawrencepj1
Copy link

…timeseries

file to register the maximum values for PCT_NAT_PFT and PCT_CFT through the time series.
This is used to determine how many PFTs and CFTs are instantiated at run time in CLM5

…timeseries

file to register the maximum values for PCT_NAT_PFT and PCT_CFT through the time series.
This is used to determine how many PFTs and CFTs are instantiated at run time in CLM5
Copy link
Member

@billsacks billsacks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for doing this @lawrencepj1 ! This basically looks very good. I have a few minor inline comments. In addition, I have one bigger comment:

In addition to outputting PCT_NAT_PFT_MAX and PCT_CFT_MAX, I think we'll also need PCT_CROP_MAX. To do this, I think you could make your new subroutine update both p2l and l2g (and then it could be renamed something more generic, like update_max_array).

@@ -511,6 +510,49 @@ end subroutine check_vals
! Module-level routines (not member functions)
! ========================================================================

!-----------------------------------------------------------------------
subroutine set_max_p2l_array(pct_pft_max_arr,pct_pft_arr)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rename "set" to "update" in this subroutine name, to indicate that it is (possibly) updating existing max values.

subroutine set_max_p2l_array(pct_pft_max_arr,pct_pft_arr)
!
! !DESCRIPTION:
! Given an array of pct_pft_type variables, set all max_p2l.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As with the comment on the subroutine name, the description should make it clear that it's (possibly) updating existing max values.

integer :: arr_index
integer :: pft_index

character(len=*), parameter :: subname = 'get_pct_p2l_array'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This parameter should be changed to match the actual subroutine name.

@@ -802,6 +806,9 @@ program mksurfdat
landfrac_pft(n) = pctlnd_pft(n)/100._r8
end do

pctnatpft_max = pctnatpft
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd find it more clear if these settings of pctnatpft_max and pctcft_max were done further down, within the conditional:

    if (mksrf_fdynuse /= ' ') then

…ut PCT_CROP_MAX in addition to

PCT_NAT_PFT_MAX and PCT_CFT_MAX. This involved modifying the new subroutine to update both p2l and l2g
and renaming it update_max_array. Additional changes were made to match Bill's suggestions of
the sub_routine subname parameter and moving the initial assignment of the max array within the
if (mksrf_fdynuse /= ' ') then code
The code has been tested and produces the maximum PCT_CROP_MAX variable as expected. PJL
Copy link
Member

@billsacks billsacks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great to me now - thanks a lot! I'll talk to @ekluzek to coordinate the next steps - generating new landuse_timeseries files, adding the code to use these fields in CTSM itself, and running testing.

@ekluzek ekluzek added this to the future milestone Apr 12, 2018
@ekluzek
Copy link
Contributor

ekluzek commented Apr 12, 2018

This should come in after the CESM2.0 release.

@ekluzek ekluzek self-assigned this Jun 26, 2018
Peter Lawrence and others added 4 commits July 3, 2018 11:42
…timeseries

file to register the maximum values for PCT_NAT_PFT and PCT_CFT through the time series.
This is used to determine how many PFTs and CFTs are instantiated at run time in CLM5
…ut PCT_CROP_MAX in addition to

PCT_NAT_PFT_MAX and PCT_CFT_MAX. This involved modifying the new subroutine to update both p2l and l2g
and renaming it update_max_array. Additional changes were made to match Bill's suggestions of
the sub_routine subname parameter and moving the initial assignment of the max array within the
if (mksrf_fdynuse /= ' ') then code
The code has been tested and produces the maximum PCT_CROP_MAX variable as expected. PJL
Begin separating SoilHydrology flux calculations

Note the new tag naming: Starting with this tag, we are naming tags
"ctsm..." rather than "clm...". We are starting with ctsm version 1,
which for now is nearly the same as clm version 5. We are moving to this
new tag naming now because:

(1) The changes in this tag represent the first step towards
    implementing the CTSM vision (separating the biogeophysics flux
    calculations from state updates, making it easier to plug in
    alternative parameterizations, etc.).

(2) This tag changes answers relative to the CLM5 release. (We expect
    the climate to be the same, but we haven't tested this carefully
    yet.)

Purpose of changes
------------------

First steps toward separating various flux calculations in the soil
hydrology code. The focus here is on saturated surface runoff and
infiltration excess runoff. The changes here separate flux calculations
from state updates and extract various calculations into their own
subroutines to facilitate swapping in alternative parameterizations.

Most of the changes here are refactorings that are either bit-for-bit or
just introduce roundoff-level differences. However, there are also a few
larger answer changes, as described below.

These are the greater-than-roundoff-level answer changes:

(A1) Use full qflx_surf in BGC code. Previously, the subroutines ch4 and
     SoilBiogeochemNLeaching had been using a flux that excluded surface
     water runoff (qflx_h2osfc_surf). That was deemed to be incorrect
     (by Dave Lawrence and others), so these BGC subroutines have been
     changed to use the full surface runoff (saturated excess runoff +
     infiltration excess runoff + h2osfc runoff).

     Configurations affected: BGC compsets

     Magnitude of change: Larger than roundoff; expected to be same
     climate, but not investigated carefully

(A2) VIC: Don't ever use the TOPModel formulation for SurfaceRunoff

     The code was using the TOPModel-based formulation for SurfaceRunoff
     if frost_table > zwt_perched. Sean Swenson felt this shouldn't be
     done, and refactoring will be easier if VIC always uses the
     vic-looking formulation, so I'm stopping applying this formulation
     when using VIC.

     Configurations affected: VIC compsets

     Magnitude of change: Larger than roundoff; not investigated carefully

(A3) VIC: Remove infiltration excess runoff

     Martyn Clark reviewed the VIC implementation, and felt that the
     current implementation of infiltration excess runoff is
     inconsistent with the standard VIC implementation. It appears that
     what was being called VIC's infiltration excess runoff was actually
     just an attempt to give a better numerical approximation to the
     solution for saturated surface excess runoff. So deleting this
     leaves only a first-order approximation to VIC's saturated surface
     excess runoff.

     Eventually we may want to put in place a more accurate solution for
     VIC's saturated surface excess runoff. But Martyn's feeling is that
     this can come in with other changes we want to make regarding
     numerical solutions in CTSM.

     Configurations affected: VIC compsets

     Magnitude of change: Larger than roundoff; expected to be same
     climate, but not investigated carefully

(A4) Change in QOVER diagnostic field: now includes QH2OSFC.

These are the major refactorings (either bit-for-bit or just
roundoff-level differences):

(R1) Extract surface runoff to its own module, and other modularization
     related to what used to be subroutine SurfaceRunoff: extract a
     subroutine for each fsat method, move urban surface runoff code
     into different routines.

(R2) Extract infiltration excess runoff to its own module, and other
     modularization related to what used to be subroutine Infiltration.

NOTE: For a detailed breakdown of changes, including documentation of
which changed changed answers, see the file ChangeLog_branch which was
deleted from this branch shortly before it was merged to master.
@ekluzek
Copy link
Contributor

ekluzek commented Jul 6, 2018

This has been added to the nfix_bugs branch, and I've done various testing on it, and it looks fine. The new variables are added in, with no changes to anything else. They have reasonable values.

@ekluzek ekluzek merged commit 59de267 into ESCOMP:master Jul 6, 2018
billsacks pushed a commit to billsacks/ctsm that referenced this pull request Feb 22, 2019
Added code to put the PCT_NAT_PFT_MAX and PCT_CFT_MAX on the landuse.…
mariuslam pushed a commit to NordicESMhub/ctsm that referenced this pull request Aug 26, 2019
Now that this has passed all tests and we understand all the reasons why it gives answer changes, pulling.  Thanks @rgknox!
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

Successfully merging this pull request may close these issues.

None yet

3 participants