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

FATES tests fail with MIMICS active #1636

Closed
slevis-lmwg opened this issue Feb 7, 2022 · 23 comments · Fixed by #1643
Closed

FATES tests fail with MIMICS active #1636

slevis-lmwg opened this issue Feb 7, 2022 · 23 comments · Fixed by #1643
Assignees
Labels
bug something is working incorrectly

Comments

@slevis-lmwg
Copy link
Contributor

Brief summary of bug

On izumi I get the error:
SoilBiogeochemDecompCascadeMIMICSMod.F90 line 852: Reference to undefined POINTER

On cheyenne the error feels more helpful:
SoilBiogeochemDecompCascadeMIMICSMod.F90 line 1111: Index '1' of dimension 1 of array 'ligninnratioavg' above upper bound of 0

ERS_D_Mmpi-serial_Ld5.1x1_brazil.I2000Clm50FatesRs.cheyenne_gnu.clm-FatesColdDef
ERS_D_Ld5.f10_f10_mg37.I2000Clm50Fates.izumi_nag.clm-FatesColdDef.0202-170916iz_nag

General bug information

CTSM version you are using: ctsm5.1.dev073-79-geba82445d

Configurations affected: soil_decomp_method = 'MIMICSWieder2015'

@slevis-lmwg slevis-lmwg added the bug something is working incorrectly label Feb 7, 2022
@slevis-lmwg slevis-lmwg self-assigned this Feb 7, 2022
@slevis-lmwg
Copy link
Contributor Author

ligninnratioavg is declared in CNVegCarbonFluxType.F90
I get the sense that we don't exercise this code when FATES is active.

@slevis-lmwg
Copy link
Contributor Author

CNDriverMod.F90 has subroutine CNDriverSummarizeFluxes where we call both soilbiogeochem_carbonflux_inst%Summary and cnveg_carbonflux_inst%Summary.

EDBGCDynMod.F90 has equivalent subroutine EDBGCDynSummary where we only call soilbiogeochem_carbonflux_inst%Summary.

@slevis-lmwg
Copy link
Contributor Author

We calculate ligninnratioavg in subr. Summary_carbonflux in module CNVegCarbonFluxType and do not call this subroutine when FATES is active, as explained in my previous post:

this%ligninNratioAvg_col(c) = &
             (ligninNratio_leaf_col(c) + ligninNratio_froot_col(c) + &
              ligninNratio_cwd) / &
              max(1.0e-3_r8, leafc_to_litter_col(c) + &
                             frootc_to_litter_col(c) + &
                             soilbiogeochem_decomp_cascade_ctransfer_col(c,i_cwdl2))

The formula uses column copies of leafc_to_litter_patch and frootc_to_litter_patch. Neither the column nor the patch copies are available with FATES active as far as I can tell. So I think we may need a refactor to make these variables available when FATES is active.

@slevis-lmwg
Copy link
Contributor Author

slevis-lmwg commented Feb 7, 2022

Though FATES doesn't do nitrogen, right? Maybe then for now we set ligninNratioAvg_col = a reasonable constant when FATES is active?

@wwieder
Copy link
Contributor

wwieder commented Feb 7, 2022

Previously I've set ligninNratioAvg to a pft specific value for C only runs. Is this a possibility for FATES too? Can we just make them hard coded or put them on the parameter file. Either way, we should clarify that the values are only used for C-only simulations.

@slevis-lmwg
Copy link
Contributor Author

Previously I've set ligninNratioAvg to a pft specific value for C only runs. Is this a possibility for FATES too? Can we just make them hard coded or put them on the parameter file. Either way, we should clarify that the values are only used for C-only simulations.

This should work. Let me know values by pft, and I will give it a try. Then we can iterate on my implementation as needed.

@wwieder
Copy link
Contributor

wwieder commented Feb 8, 2022

I think we can start with lignin and N values from Table 1 in this paper. Let me know if there's uncertainty in how to map the pfts listed here to the CLM pfts
https://bg.copernicus.org/articles/9/565/2012/bg-9-565-2012.pdf

@slevis-lmwg
Copy link
Contributor Author

Thanks @wwieder, I will look into this today.

I wanted to include a helpful comment @rgknox made at stand-up yesterday:
Beyond the above fix intended for when use_fates = .false., @rgknox said that getting MIMICS to work with FATES will require a bigger refactor. @rgknox recommended not worrying about it at this time.

@rgknox pls correct me if I misunderstood.

@slevis-lmwg
Copy link
Contributor Author

Beyond the above fix intended for when use_fates = .false., @rgknox said ...

Correction: The above fix is for when use_fates = .true. and it leaves the more complex refactor for later.

@slevis-lmwg
Copy link
Contributor Author

@wwieder
we also need values by pft for annsum_npp_col because this is also declared in CNVegCarbonFluxType.F90

@rgknox
Copy link
Collaborator

rgknox commented Feb 9, 2022

cc'ing @ckoven and @rosiealice
@slevisconsulting and @wwieder , that is my sense, but I need to learn more about mimics and what types of boundary conditions it requires from FATES. I also shouldn't assume what work and efforts have already been done to enable compataibiity between these two models.
In the stand-up meeting, I was alluding that for FATES coupling with century (its still century right? :/) and existing decomposition soil BGC, we bypass some code on the CLM side that would typically be run when use_cn is true, and instead use a simplified version of the code maintained in a file called EDBGCDynMod.F90. Ideally, we would want FATES to coordinate with all of the native code for decomposition (and nutrient dynamics) on the CLM side, without having our own simpflified (or different) code to handle these things. I think it also would be good to walk through the sequence of physics events, in all things BGC (decomp and nutrient), and re-evaluate how fates needs to accomodate it and vice-versa.. maybe the key phrase here is wholistic approach?
Yeah, so the broader refactor I was thinking of would touch on this last point. To avoid the code-tacked-on-code apacolypse, it seems a good idea to consider fates connection with mimics at the same time we refactor fates-century/existing bgc model. Does that make sense?

@ckoven
Copy link
Contributor

ckoven commented Feb 9, 2022

@wwieder agreed with @rgknox we should sit down and discuss what MIMICS needs to know about its litter inputs and figure out how to reintegrate FATES with nutrients into the full sets of decomposition schemes rather than the current way that bypasses all the nutrient code.

@wwieder
Copy link
Contributor

wwieder commented Feb 9, 2022

Agreed, this is a good conversation to have.
What kind of timeline makes sense for this? I ask because MIMICS is functional with CTSM-BGC, but still need work related to spinup that will actually make it scientifically viable. I also need to spend more time evaluating the current configuration of the model. In the short term should we just not allow a MIMICS-FATES simulation to even occur, or are there smaller things we should consider now on the MIMICS /HLM side that we should be considering now for FATES?

@ckoven
Copy link
Contributor

ckoven commented Feb 9, 2022

@wwieder I guess it'd be useful to know if there is anything more granular that MIMICS wants to know about its litter inputs than we currently resolve, just so that we can figure out how to resolve that when the time comes. Since nobody is currently dedicating time to connecting the FATES nutrient code to CLM, I think it can wait, and yes, we should probably disallow running FATES with mimics. And then I'd recommend that once a dedicated effort is made to reconnect FATES to CLM with nutrients, that we make sure that effort does so in a MIMICS-consistent way.

@slevis-lmwg
Copy link
Contributor Author

slevis-lmwg commented Feb 9, 2022

@wwieder I guess it'd be useful to know if there is anything more granular that MIMICS wants to know about its litter inputs than we currently resolve, just so that we can figure out how to resolve that when the time comes. Since nobody is currently dedicating time to connecting the FATES nutrient code to CLM, I think it can wait, and yes, we should probably disallow running FATES with mimics. And then I'd recommend that once a dedicated effort is made to reconnect FATES to CLM with nutrients, that we make sure that effort does so in a MIMICS-consistent way.

The quick answer to what FATES-MIMICS tests need to pass is calculations of:
ligninNratioAvg and annsum_npp_col, both currently declared and calculated in CNVegCarbonFluxType.F90.

ligninNratioAvg is a function of leafc_to_litter_patch, frootc_to_litter_patch, soilbiogeochem_cwdc_col, soilbiogeochem_cwdn_col, soilbiogeochem_decomp_cascade_ctransfer_col.

I got both FATES and carbon_only tests to pass by declaring local copies (in MIMICS) of ligninNratioAvg and annsum_npp_col. This means that we could, for now, have FATES share the carbon_only code that I'm adding.

I will open a PR with my initial modifications soon. After you see my changes, let me know if it's still preferable to disallow running FATES with MIMICS.

@rosiealice
Copy link
Contributor

This is great @slevisconsulting @wwieder . I had no idea this work was so far along. Do we need to consider the minimum set of what mimics passes back to fates @rgknox or is that perhaps downstream of the basic parteh/fates coupling?

@wwieder
Copy link
Contributor

wwieder commented Feb 11, 2022

@slevisconsulting never in a million years did I think that we'd couple a C only version of MIMICS with FATES so quickly.
This is potentially very exciting.

@rosiealice for PARTEH here are my ideas.
Short term: I'm hoping that any information that FATES is getting from the HLM related to soil biogeochemistry (inorganic N pools) will not be changed by running MIMICS. There may be some complications related to spinup that we need to consider...

Medium term: I'm imagining there will need to be some logic for how we handle N competition that will be tricky and somewhat HLM dependent? Are you targeting an ELM configuration to develop this with? If so maybe CLM should mirror this approach to facilitate the FATES-HLM coupling, as our current sequential demand-based approach leaves much to be desired?

Long term: Ideally, we'd be able to come up with a clever way for FATES to simulate below ground allocation, especially related to root exudation, that would modify soil biogeochemical behavior and N mineralization rates. This is a pretty ambitious project, however, both scientifically and on the SE side that would likely take some coordination and funding.

@rosiealice
Copy link
Contributor

Hi Will,

This all sounds sensible. I'll defer to @rgknox on the details of the ELM configuration.

Noting that Terje Berntsen has just got a big soil BGC project funded in Oslo. Also in the long term, it might be useful to coordinate with him and Elin on any developments they are planning to avoid reinventing the wheel. I will try and figure this out.

@rgknox
Copy link
Collaborator

rgknox commented Mar 4, 2022

Just a note here that when we do fully couple MIMICS & FATES with N cycling, I think we need to pass ligninNratioAvg from MIMICS to FATES via the FATES bc_in structure. Do I have that right? MIMICS calculates a fraction of N in lignins, instead of leaving it a constant? And we need to pass that info to FATES so it generates the litter fractions accordingly.

@wwieder
Copy link
Contributor

wwieder commented Mar 4, 2022

MIMICS calculated litter quality as a ratio of lignin:N in total litterfall. Where this gets complicated, especially with FATES, is with CWD fluxes. The important information to pass to MIMICS would be column averaged N and lignin (as a fraction of total litterfall) fluxes? Does this answer your question, @rgknox?

@rgknox
Copy link
Collaborator

rgknox commented Mar 4, 2022

yeah, so I had it backwards. Its FATES' job to calculate the column level C:N ratio of lignin tissues and pass that to MIMICS.

One more question though: Are any organs excluded from this average? I see in the CLM code that it diagnoses this ratio from leaves, fine-roots and CWD. Like, for instance in fates, we have small branches that would contribute to fine litter pools but not CWD. Or should we just perform a mass weighted average of our C:N ratio for all lignin tissues in the litter pool?

@wwieder
Copy link
Contributor

wwieder commented Mar 4, 2022

It's even simpler that that, @rgknox, the C:N ratio of all litterfall and the lignin concentration of all litterfall. MIMICS then calculates lignin:N to use internally.

The wood classes in FATES introduce a complication here. You handle your own decomposition of branch classes internally, right, with the collective output going back to structural litter in the HLM?

If so, I think this is what MIMICS should be counting from CLM already (it's actually the litter quality associated with the flux of CWD_TO_LITTER that's being used for the fMET calculation in MIMICS). Can you confirm this @slevisconsulting?
but see also

ligninNratio_cwd = CNParamsShareInst%cwd_flig * &

If this is accurate, we'd similarly need the aggregate information of total C, total N, and lignin fraction of the analogous (lumped) CWD_TO_LITTER flux from FATES.

@slevis-lmwg
Copy link
Contributor Author

(it's actually the litter quality associated with the flux of CWD_TO_LITTER that's being used for the fMET calculation in MIMICS). Can you confirm this @slevisconsulting?

I hope this isn't redundant. I will show formulas, because I'm not fluent in the terminology...
fmet = mimics_fmet_p1 * (mimics_fmet_p2 - mimics_fmet_p3 * min(mimics_fmet_p4, ligninNratioAvg_scalar))
...and the calculation of ligninNratioAvg

! Calculate ligninNratioAve
       do fc = 1,num_soilc
          c = filter_soilc(fc)
          if (soilbiogeochem_cwdn_col(c) > 0._r8) then
             ligninNratio_cwd = CNParamsShareInst%cwd_flig * &
                (soilbiogeochem_cwdc_col(c) / soilbiogeochem_cwdn_col(c)) * &
                soilbiogeochem_decomp_cascade_ctransfer_col(c,i_cwdl2)
          else
             ligninNratio_cwd = 0._r8
          end if
          this%ligninNratioAvg_col(c) = &
             (ligninNratio_leaf_col(c) + ligninNratio_froot_col(c) + &
              ligninNratio_cwd) / &
              max(1.0e-3_r8, leafc_to_litter_col(c) + &
                             frootc_to_litter_col(c) + &
                             soilbiogeochem_decomp_cascade_ctransfer_col(c,i_cwdl2))
       end do

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something is working incorrectly
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants