Create driver data structure for SP mode #2952

adrifoster · 2025-02-05T21:25:23Z

Description of changes

Creates a new data structure for htop, tlai, and tsai that is just driver data.

Specific notes

Previously, fates used htop_patch (etc.) as a driver data, but we also needed to update it and set set htop_hist_patch. This htop_hist_patch was inconsistently used throughout the CLM code, resulting in mis-matches between the PFT structures that we actually were trying to run (see #2945).

Now we have the SP driver data as a separate data structure, and methods to set it. FATES sets it and passes it back to CLM through the bc_out struct, CLM just sets it in a 1:1 call.

Contributors other than yourself, if any:

@ekluzek @rgknox @rosiealice

CTSM Issues Fixed (include github issue #): Closes #2945

Are answers expected to change (and if so in what way)? Yes - because we were incorrectly using it before, all FATES-SP compsets should change.

ALL OTHER COMPSETS SHOULD BE B4B

Any User Interface Changes (namelist or namelist defaults changes)? No

Does this create a need to change or add documentation? Did you do so? N/A

Testing performed, if any:

Will test

src/biogeochem/SatellitePhenologyMod.F90

adrifoster · 2025-02-06T20:20:26Z

Testing aux_clm on derecho:

Removed expected fails...

0206-092821de_gnu: 65 tests
FAIL SMS_D.1x1_brazil.I2000Clm60FatesSpCruRsGs.derecho_gnu.clm-FatesColdDryDepSatPhen BASELINE ctsm5.3.021: DIFF
FAIL SMS_D.1x1_brazil.I2000Clm60FatesSpCruRsGs.derecho_gnu.clm-FatesColdMeganSatPhen BASELINE ctsm5.3.021: DIFF

0206-092821de_int: 167 tests
FAIL ERP_P128x2_Ld30.f45_f45_mg37.I2000Clm60FatesSpCruRsGs.derecho_intel.clm-FatesColdSatPhen COMPARE_base_rest
FAIL ERP_P128x2_Ld30.f45_f45_mg37.I2000Clm60FatesSpCruRsGs.derecho_intel.clm-FatesColdSatPhen BASELINE ctsm5.3.021: DIFF

0206-092821de_nvh
FAIL SMS_D.f10_f10_mg37.I2000Clm60BgcCrop.derecho_nvhpc.clm-crop RUN time=35
FAIL SMS.f45_f45_mg37.I2000Clm60FatesSpRsGs.derecho_nvhpc.clm-FatesColdSatPhen BASELINE ctsm5.3.021: DIFF

Does anyone (@samsrabin?) know why SMS_D.f10_f10_mg37.I2000Clm60BgcCrop.derecho_nvhpc.clm-crop RUN time=35 ? The log files don't really show anything at all. Only the med.log and cesm.log have anything in them at all.

ekluzek · 2025-02-07T16:05:12Z

Does anyone (@samsrabin?) know why SMS_D.f10_f10_mg37.I2000Clm60BgcCrop.derecho_nvhpc.clm-crop RUN time=35 ? The log files don't really show anything at all. Only the med.log and cesm.log have anything in them at all.

Check the baseline and see if it died as well and didn't get put into expected fails.

We've been having problems with the nvhpc compiler, so personally I'm not too inclined to make sure it works if everything else is OK. But, always good to spend some time looking at it...

ekluzek

This is great, this improves the current code structure and removes some problematic logic in the code. I have a few questions on names, that we can discuss.

As a separate thing I'd like us to be thinking about what kind of design we'd like to have for FATES-SP that we can work towards. If we do some thinking there we can make sure steps like this are moving us toward what we think the end design goal might be. I think that should probably be an issue itself, where we put some design thinking in. So we should open that and reference here. This would just be thought of as a step toward that, and not doing everything desired there.

Happy to discuss and do more thinking about naming and a design goal to work towards.

But, thanks for the work here, I like that you are both fixing an immediate problem and improving the code. That's the golden thing to try to do when we can.

src/biogeochem/SatellitePhenologyMod.F90

src/biogeophys/CanopyStateType.F90

src/utils/clmfates_interfaceMod.F90

adrifoster · 2025-02-07T16:11:02Z

Does anyone (@samsrabin?) know why SMS_D.f10_f10_mg37.I2000Clm60BgcCrop.derecho_nvhpc.clm-crop RUN time=35 ? The log files don't really show anything at all. Only the med.log and cesm.log have anything in them at all.

Check the baseline and see if it died as well and didn't get put into expected fails.

We've been having problems with the nvhpc compiler, so personally I'm not too inclined to make sure it works if everything else is OK. But, always good to spend some time looking at it...

There is no baseline for this because it was not supposed to pass SHAREDLIB_BUILD (has an expected fail for that), but somehow it passed that stage but died in RUN

src/biogeochem/SatellitePhenologyMod.F90

adrifoster · 2025-02-18T20:37:07Z

@ekluzek, @rosiealice, or anyone...

we have encountered some further issues.

It seems that we will need some further refactoring to get this bug fixed.

I spoke with @glemieux and he pointed out that CanopyFluxes runs before we ever get to the fates call that sets up the satellite phenology structures. Thus, even if we have fates pass back the correct htop array, it will not ever get used.

Is this correct?

rosiealice · 2025-03-03T11:40:52Z

Do you mean, not 'ever' get used, or just not get used until FATES is called at the end of the day? I guess either way this is problematic/will cause restart test failures. I guess somehow we need to pass through the SP code during initialization to fix this?

ekluzek · 2025-03-10T17:19:06Z

src/biogeochem/SatellitePhenologyMod.F90

@@ -24,7 +24,8 @@ module SatellitePhenologyMod
  private
  !
  ! !PUBLIC MEMBER FUNCTIONS:
-  public :: SatellitePhenology     ! CLMSP Ecosystem dynamics: phenology, vegetation
+  public :: GetSatellitePhenologyInputs ! put the data into the correct format
+  public :: SetSatellitePhenologyBGCCanopyStructs ! CLM(BGC)-SP phenology and vegetation


I'm just wondering about changing the name for the Set method to something like...

UpdateSatellitePhenologyCanopy

I think of Set methods as doing nothing outside of setting variables. This does a bit more than that.

I see what you are doing with adding BGC to distinguish it from FATES, but we've used BGC exclusively to refer to "use_cn==TRUE". So I'm not sure what to do here.

sure sounds good

Compsets: Sp, Bgc, BgcCrop, Fates, FatesSp

ekluzek

@adrifoster thanks for your nice work here and the cleanup of the code calling structure. I just have some thoughts on names of things, which is always a hard thing to get right. I have some suggestions and we can talk about this a bit when we meet in a little bit.

ekluzek · 2025-03-10T20:24:09Z

src/biogeochem/SatellitePhenologyMod.F90

+    real(r8) :: fb                                ! fraction of canopy layer covered by snow
+
+    if (use_fates_sp) then 
+      write(iulog,*) 'SetSatellitePhenologyBGCCanopyStructs', 'should not be calling this method when use_fates_sp == .true.'


Remove the subroutine name. The errMsg serves this purpose. See #1452

It's actually for any FATES simulation, so maybe should be use_fates?

And actually maybe this check should just be done at initialization? But, since it's a subroutine call -- there isn't a way to do that. Hmmm.... A single check like this is fine

src/biogeophys/CanopyStateType.F90

src/biogeochem/SatellitePhenologyMod.F90

ekluzek · 2025-03-10T20:41:15Z

src/biogeochem/SatellitePhenologyMod.F90

@@ -24,7 +24,8 @@ module SatellitePhenologyMod
  private
  !
  ! !PUBLIC MEMBER FUNCTIONS:
-  public :: SatellitePhenology     ! CLMSP Ecosystem dynamics: phenology, vegetation
+  public :: GetSatellitePhenologyInputs ! put the data into the correct format


I kind of think of a method with Get in the title as a getter method. And this is more than that. We tend to use Calc or Update for something more extensive. This is really where the interpolation is applied though.

So maybe something like...

CalcSatellitePhenologyTimeInterp?

adrifoster added 4 commits February 5, 2025 13:51

make new canopystate_inst variable

4575646

update to use new array

0cc6c76

update calls

cf52947

fix typos

8b02eb7

wwieder reviewed Feb 6, 2025

View reviewed changes

src/biogeochem/SatellitePhenologyMod.F90 Outdated Show resolved Hide resolved

ekluzek reviewed Feb 7, 2025

View reviewed changes

update names; add endrun

904b184

adrifoster changed the title ~~[WIP] Create driver data structure for SP mode~~ Create driver data structure for SP mode Feb 7, 2025

wwieder assigned adrifoster Feb 13, 2025

adrifoster added 2 commits February 13, 2025 10:50

update names

f0f4c1c

Merge branch 'master' into fix_fates_sp

23ad050

samsrabin mentioned this pull request Feb 15, 2025

Change one derecho_nvhpc.clm-crop test to expected fail in RUN #2968

Open

samsrabin added enhancement new capability or improved behavior of existing capability test: aux_clm Pass aux_clm suite before merging test: fates Pass fates test suite before merging labels Feb 16, 2025

update

7dc5a09

glemieux reviewed Feb 18, 2025

View reviewed changes

src/biogeochem/SatellitePhenologyMod.F90 Show resolved Hide resolved

adrifoster and others added 7 commits February 18, 2025 15:26

testing

9cb42ec

fix

90a9dc9

update

88ab16e

back to original

b352cbb

fix typo

49e5437

remove unused vars

2bd496b

Merge branch 'master' into fix_fates_sp

c4b50d7

ekluzek reviewed Mar 10, 2025

View reviewed changes

ekluzek requested changes Mar 10, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create driver data structure for SP mode #2952

Create driver data structure for SP mode #2952

adrifoster commented Feb 5, 2025

adrifoster commented Feb 6, 2025

ekluzek commented Feb 7, 2025

ekluzek left a comment

adrifoster commented Feb 7, 2025

adrifoster commented Feb 18, 2025

rosiealice commented Mar 3, 2025

ekluzek Mar 10, 2025 •

edited

Loading

adrifoster Mar 10, 2025

ekluzek Mar 10, 2025

ekluzek left a comment

ekluzek Mar 10, 2025

ekluzek Mar 10, 2025

ekluzek Mar 10, 2025

Create driver data structure for SP mode #2952

Are you sure you want to change the base?

Create driver data structure for SP mode #2952

Conversation

adrifoster commented Feb 5, 2025

Description of changes

Specific notes

adrifoster commented Feb 6, 2025

ekluzek commented Feb 7, 2025

ekluzek left a comment

Choose a reason for hiding this comment

adrifoster commented Feb 7, 2025

adrifoster commented Feb 18, 2025

rosiealice commented Mar 3, 2025

ekluzek Mar 10, 2025 • edited Loading

Choose a reason for hiding this comment

adrifoster Mar 10, 2025

Choose a reason for hiding this comment

ekluzek Mar 10, 2025

Choose a reason for hiding this comment

ekluzek left a comment

Choose a reason for hiding this comment

ekluzek Mar 10, 2025

Choose a reason for hiding this comment

ekluzek Mar 10, 2025

Choose a reason for hiding this comment

ekluzek Mar 10, 2025

Choose a reason for hiding this comment

ekluzek Mar 10, 2025 •

edited

Loading