FATES cases fail when using crop datasets

[ekluzek]

Brief summary of bug

FATES cases fail when you point to surface datasets with crop rather than the 16pft versions.

General bug information

CTSM version you are using: ctsm5.1.dev057-11-g96d6865e7

Does this bug cause significantly incorrect results in the model's science?No

Configurations affected: FATES cases

Details of bug

Following cases fail when pointing to surface datasets with crop on them:

ERS_D_Ld5.f09_g17.I2000Clm45Fates.cheyenne_intel.clm-FatesColdDef.GC.ctsm51d58acl_int ERS_D_Ld5.f09_g17.I2000Clm50Fates.cheyenne_intel.clm-FatesColdDef.GC.ctsm51d58acl_int ERS_D_Ld5.f10_f10_mg37.I2000Clm45Fates.cheyenne_intel.clm-FatesColdDef.GC.ctsm51d58acl_int ERS_D_Ld5.f10_f10_mg37.I2000Clm50Fates.cheyenne_intel.clm-FatesColdDef.GC.ctsm51d58acl_int ERS_Ld9.f10_f10_mg37.I2000Clm50FatesCru.cheyenne_intel.clm-FatesColdDefCH4.GC.ctsm51d58acl_int SMS_D_Ld5.f10_f10_mg37.I2000Clm45Fates.cheyenne_gnu.clm-FatesColdDef.GC.ctsm51d58acl_gnu SMS_D_Ld5.f10_f10_mg37.I2000Clm45Fates.cheyenne_intel.clm-FatesColdDef.GC.ctsm51d58acl_int SMS_D_Ld5.f10_f10_mg37.I2000Clm50Fates.cheyenne_gnu.clm-FatesColdDef.GC.ctsm51d58acl_gnu SMS_D_Ld5.f10_f10_mg37.I2000Clm50Fates.cheyenne_intel.clm-FatesColdDef.GC.ctsm51d58acl_int SMS_D_Ld5.f45_f45_mg37.I2000Clm45Fates.cheyenne_intel.clm-FatesColdDef.GC.ctsm51d58acl_int SMS_D_Ld5.f45_f45_mg37.I2000Clm51Fates.cheyenne_intel.clm-FatesColdDef.GC.ctsm51d58acl_int SMS_D_Lm6.f45_f45_mg37.I2000Clm45Fates.cheyenne_intel.clm-FatesColdDef.GC.ctsm51d58acl_int SMS_D_Lm6.f45_f45_mg37.I2000Clm50Fates.cheyenne_intel.clm-FatesColdDef.GC.ctsm51d58acl_int SMS_D_Lm6_P144x1.f45_f45_mg37.I2000Clm50Fates.cheyenne_intel.clm-FatesColdDef.GC.ctsm51d58acl_int SMS_Ld5.f10_f10_mg37.I2000Clm45Fates.cheyenne_intel.clm-FatesColdDef.GC.ctsm51d58acl_int SMS_Ld5.f10_f10_mg37.I2000Clm50Fates.cheyenne_gnu.clm-FatesColdDef.GC.ctsm51d58acl_gnu SMS_Ld5.f10_f10_mg37.I2000Clm50Fates.cheyenne_intel.clm-FatesColdDef.GC.ctsm51d58acl_int SMS_Ld5.f19_g17.I2000Clm45Fates.cheyenne_intel.clm-FatesColdDef.GC.ctsm51d58acl_int SMS_Ld5.f19_g17.I2000Clm50Fates.cheyenne_intel.clm-FatesColdDef.GC.ctsm51d58acl_int

Important details of your setup / configuration so we can reproduce the bug

SMS_D_Ld5.f10_f10_mg37.I2000Clm50Fates.cheyenne_intel.clm-FatesColdDef

fsurdat = '/glade/p/cesmdata/cseg/inputdata/lnd/clm2/surfdata_map/release-clm5.0.18/surfdata_10x15_hist_78pfts_CMIP6_simyr2000_c190214.nc'

Important output or errors that show the problem

 Attempting to read surface boundary data .....
 (GETFIL): attempting to find local file 
 surfdata_10x15_hist_78pfts_CMIP6_simyr2000_c190214.nc
 (GETFIL): using 
 /glade/p/cesmdata/cseg/inputdata/lnd/clm2/surfdata_map/release-clm5.0.18/surfda
 ta_10x15_hist_78pfts_CMIP6_simyr2000_c190214.nc
 check_var: variable xc is not on dataset
 surfrd_get_data lon_var = LONGXY lat_var =LATIXY
 WARNING: New CFT-based format surface datasets should be run with 
 create_crop_landunit=T
 WARNING: When fates is on we allow new CFT based surface datasets 
 to be used with create_crop_land FALSE
 check_dim_size ERROR: mismatch of input dimension           64 
  with expected value            2  for variable cft
 ERROR: 
 ERROR in /glade/work/erik/ctsm_worktrees/misc_PPE_answer_changes/src/main/ncdio
 _pio.F90.in at line 409

[ekluzek]

I think has to do with the fact that FATES runs with create_crop_landunit=FALSE, more than anything about FATES itself.

[ekluzek]

This has to do with this code in surfrdMod.F90:

       else if ( .not. create_crop_landunit )then
          if ( masterproc ) write(iulog,*) "WARNING: New CFT-based format surface datasets should be run with ", &
                                           "create_crop_landunit=T"
          if ( use_fates ) then
             if ( masterproc ) write(iulog,*) "WARNING: When fates is on we allow new CFT based surface datasets ", &
                                              "to be used with create_crop_land FALSE"
             cftsize = actual_numcft
             allocate(array2DCFT (begg:endg,cft_lb:cftsize-1+cft_lb))
             allocate(array2DFERT(begg:endg,cft_lb:cftsize-1+cft_lb))
             call surfrd_cftformat( ncid, begg, endg, array2DCFT, array2DFERT, cftsize, natpft_size-cftsize ) ! Read crops in as CFT's
             call convert_cft_to_pft( begg, endg, cftsize, array2DCFT )                          ! Convert from CFT to natural veg. landunit
             fert_cft(begg:,cft_lb:) = 0.0_r8
          else
             call endrun( msg=' ERROR: New format surface datasets require create_crop_landunit TRUE'//errMsg(sourcefile, __LINE__))
          end if

I can see how to fix the above, but then cft_size/cft_ub is wrong in clm_varpar which causes problems in clmfates_interfaceMod.

In the FATES case I think it's important for create_crop_landunit to be off. So we need to figure this out. We now have separate code that take crop surface datasets and knocks it down to generic crops, so maybe that's the code that should be used rather than this special code here. We should be able to leave those generic crops on the same vegetated landunit.

@ckoven @rgknox @rosiealice is it important in FATES for create_crop_landunit to be off? This means that generic crops for fates are on the same natural vegetated landunit as other natural vegetation. I suppose what FATES is really doing is just using the space for generic crops to run all of the natural veg over it and not really doing anything with crop. As such it is probably critical to keep it this way. But, I want to confirm with you all.

[rgknox]

@ekluzek fates thus far has been coupled in such a way to not allow coexistance with crops. This choice was for simplicity. Part of this reason is because we have various code structures in place where:

if(use_fates)then
  do fates stuff
else
  loop through a patch filter
end

So the problem is that fates excludes looping through various patch filters (because fates would have no analogue to some processes that happen on the pft (patch) scale), and presumably these patch/pft structures would hold crops on them. I don't have a good sense of how ubiquitous this type of exclusion is in the code, and there are likely other issues preventing the two models from running simultaneously.

But, at some point we need to start breaking from the use_fates sledgehammer, and start using the is_fates column-level and patch-level filters.

Does this help at all?

Is there any reason to have create_crop_landunit to be on and not have crops running? (Im assuming they come as a package)

[rosiealice]

Hi @ekluzek

Interesting question. A relevant discussion is here https://github.com/NGEET/fates/issues/760 where we thought that we might indeed try and create a seperate instance of FATES on the crop landuit while in SP mode. Thus, we wouldn't want to make it so that this is never possible... (I am planning to implement this as part of follow-ups to the SP implementation).

[ekluzek]

OK good, thanks for the replies. Yes, that confirms what I thought. So we need it to work this right now, but can add more flexibility when first we use fates filters everywhere in CTSM and secondly (as @rosiealice points out) when FATES is changed to allow it to run over crop landunit types. As a secondary thing to that we'll probably want to add two new landunit types for FATES one for natural veg (to distinguish FATES natural veg from CLM natural veg) and a FATES crop one (with similar reasoning).

This means I need to study this code more carefully and make sure we can do what is needed right now, while also allowing our future plans to happen relatively easy as well.

[rosiealice]

Sounds good @ekluzek. I'm not sure of the use-case where we have both CLM5 and FATES natural veg active at the same time? But maybe you are just covering all the bases?

[billsacks]

Discussion in ctsm software meeting: It may not be worth fixing this, since the long-term solution is to make fates work with create_crop_landunit true (which is what we want in general moving forward). So in the short-term, maybe either live with needing to create additional datasets, or continue to use the old surface datasets with fates.

[ekluzek]

In looking into this it looks to me like it should work, with a one line change. But, it's not as there seems to be an off by one array size problem in FATES. I think it's subtle enough that it's likely going to take quite a bit of work to resolve.

I also note that the crop collapse CFT methods only work for the create_cropland=T case, so it doesn't help for FATES. They could be reworked to apply for the create_cropland=F case, but it hasn't been worked out yet. I also think that running with crop for FATES might increase memory in a useless way, so getting it to work might not be that great from that angle either.

So I'm going to ignore this problem for now, and make FATES require using 16PFT surface datasets. This was a conclusion we came to at our software meeting, where we decided putting too much time into this might not be worth it.

[ekluzek]

There are now arrays is_fates that tell you if a column is fates or not, as @rgknox talked about above. It might be worth getting this working for at least single point simulations (because of #1609). It might be reasonable to get this working with just checking that all veg points have is_fates on, and if they don't to abort at that point.

[ekluzek]

@rgknox and I discussed this last Friday. Even for FATES SP mode crops are ignored by FATES. So we really don't need to even give FATES the generic crops even for the SP case. The question is what to do with the crop area though.

[rgknox]

@ekluzek , following our conversation, I worked more on the ability of FATES to dictate natural-veg patch counts, via: https://github.com/ESCOMP/CTSM/pull/1766

I think with that PR, it should be no problem to read in surface files with CropFT's, and have fates SP patches represent those CFTs.

If we want to have FATES running alongside the crop model (on its own LU), that work still needs to be done.

[wwieder]

Maybe we can talk next week at the SE meeting, but why doesn't FATES-sp run crop areas as generic grasses, just like the HLM-sp version of the model does? This would avoid different LAIs that are seen in FATES vs. CTSM sp simulations?

[ekluzek]

@wwieder yes that's what we plan to have setup. There's some setup in the FATES parameter file that we need to have done out of the box. So I think we can set it up that way. I need to look over @rgknox PR above more to see how close it gets us to this. I'm also not clear if irrigation will work correctly here. I'm meeting with @rgknox and @glemieux about this tomorrow, if anyone else wants to join -- let us know. Otherwise, we'll update everyone next week, and on github.

[wwieder]

OK, thanks @ekluzek . I didn't think about the complication of irrigation... I trust you all on this and am happy to wait for the update next week.

[rgknox]

In my PR I'm assuming that irrigation and fertilizer is turned off, the datasets can't be used with fates. That could be a future stretch goal.

[adrifoster]

@ekluzek do we want to try to fix this this week? I can try to work on it...

[rgknox]

@adrifoster, my understanding (and the intent of recent PR https://github.com/ESCOMP/CTSM/pull/1766), was that FATES should not fail anymore when using expanded CROP surface datasets. The only issue is that if we want to test fates with the expanded crop surface datasets, we need to update the fates parameter file, so that the fates pft to clm cft mapping table is expanded to handle all 64 plant types. Although, we have not done much testing on this front, so there may be lingering issues.

[ckoven]

If we do that, would it create an inconsistency with the ELM PFT mapping?

[rgknox]

@ckoven I think it depends on how we implement this. But like you say, the ELM and CLM surface datasets are all different, so the mapping we have in our fates parameter file needs to be catered to each specific surface file if we run tests that use biogeography.

[rosiealice]

Noting that in my runs when expanded the FATES to HLM PFT mapping to include crops there was a round off error. I fixed this in my code but I'm on an island today so will submit a PR for the fix hopefully tomorrow or Wednesday.

[adrifoster]

@rgknox do we have a good method for mapping the crop types? are they all just c3 grasses?

[rgknox]

I believe the default approach is to map to grass for all crops. It makes sense to me to match the photosynthetic pathway as well.

[ckoven]

@rgknox do we have a good method for mapping the crop types? are they all just c3 grasses?

I think this overlaps significantly with how we think about specifying a FATES crop PFT. Another option is that we more fully decouple the FATES nocomp logic away from the surface dataset file and towards the FATES land-use file, thus reducing the dependencies between FATES and the surface dataset. Some discussion on this point on an upcoming call might be helpful.

[rgknox]

Interesting idea @ckoven

[adrifoster]

@ckoven agree, perhaps though initially we can do the first option (just so we can get things functional)? and then move towards the second one.

[ckoven]

@adrifoster makes sense to me.