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_PATCHAREA_AP only has data in chronologically-first history file #1207

Closed
samsrabin opened this issue May 23, 2024 · 12 comments · Fixed by #1208
Closed

FATES_PATCHAREA_AP only has data in chronologically-first history file #1207

samsrabin opened this issue May 23, 2024 · 12 comments · Fixed by #1208
Labels
bug - software engineering history output Pertaining to FATES history output variables

Comments

@samsrabin
Copy link
Contributor

As of sci.1.73.0_api.35.0.0, the per-age-class patch area output FATES_PATCHAREA_AP only has data saved in the first (chronologically) history output file. The variable itself is there, but full of missing values.

This didn't happen in sci.1.71.0_api.33.0.0. I can't test any intervening tags because those are the closest ones with which we have CTSM working.

This blocks my ability to work on #1205.

@samsrabin
Copy link
Contributor Author

I suspect this is related to the change of upfreq in the call this%set_history_var(vname='FATES_PATCHAREA_AP', ... call from 1 to group_dyna_complx (which is set to 2) as part of @rgknox's history density updates. Ryan, would that have done it?

@samsrabin
Copy link
Contributor Author

Confirming that switching it back to upfreq=1 fixes things.

@rgknox
Copy link
Contributor

rgknox commented May 23, 2024

ok, looking into this now

@samsrabin
Copy link
Contributor Author

samsrabin commented May 23, 2024

Some questions:

  1. Is this expected behavior for upfreq=group_dyna_complex (i.e., 2)?
  2. If so, it seems like there are lots of variables that got this same change. Do any of those need to be rolled back?
  3. If not, what testing can be added to ensure that no accidental changes like this happen in the future?
  4. Is there any documentation on the history density options?

samsrabin added a commit to samsrabin/fates that referenced this issue May 23, 2024
Works around NGEET#1207: FATES_PATCHAREA_AP only has data in chronologically-first history file.
@rgknox
Copy link
Contributor

rgknox commented May 23, 2024

Here is the setting described on the wiki page: https://fates-users-guide.readthedocs.io/en/latest/user/Namelist-Options-and-Run-Time-Modes.html

@samsrabin can you confirm that fates_history_dimlevel = 2,2 ?

@rgknox
Copy link
Contributor

rgknox commented May 23, 2024

or is use_fates_ed_st3 = True ?

@samsrabin
Copy link
Contributor Author

I used out-of-the-box settings for those:

  • fates_history_dimlevel = 2,2
  • use_fates_ed_st3 = .false.

@samsrabin
Copy link
Contributor Author

I'm also not seeing anything in the docs about group_dyna_whatever. upfreq shows up in one place but the documentation seems outdated because it doesn't mention the new group_dyna settings. I guess fates_history_dimlevel has to do with it though?

@rgknox
Copy link
Contributor

rgknox commented May 23, 2024

Well, the group_dyna settings are not forward facing to the user, so that is not why it is in the users guide. Maybe better commenting in the code is in order.

group_dyna_simple and group_dyna_complx are used to flag history variables that are updated on the dynamics step, and are a single value for the column (simple) or arrays (complx) respectively. The simple group is active if fates_history_dimlevel(2) > 0, and the complex group is active if fates_history_dimlevel(2)>1.

Still trying to figure out why this is not working.

Could you try adding the following line:

hio_area_si_age(io_si,:) = 0._r8

At the top of the site loop before anything else happens, perhaps around this line:
https://github.com/NGEET/fates/blob/sci.1.76.3_api.35.1.0/main/FatesHistoryInterfaceMod.F90#L3283

@samsrabin
Copy link
Contributor Author

Yup, that also fixes the issue.

@rgknox
Copy link
Contributor

rgknox commented May 23, 2024

One of the issues here is that with these multi-dimension output variables, there may be indices in these arrays that are invalid.

For instance, what if you wanted to report leaf temperatures on an array that has a second dimension that spans leaf-layers, but since the crowns are only so deep, you only have leaves on the first several leaf layer indices. It would be most straight forward to zero the output array, loop through the cohorts, increment the temperatures on the leaf-layers that are encountered, and then normalize. But then the issue is that we have zero's in the unused indices, which is misleading. Its not zero degrees, it should be invalid because there are no leaves.

We have a method that initializes all variables in a group to zero, but I'm not a fan of it for the previous reason. Also, there are plenty of output variables that are filled without incrementing (just uses assignment), so zero'ing them is unecessary and inefficient. I only like zero'ing something when there is a specific need to zero it (such as incrementing to create a mean, or something like that).

I see other variables that are being incremented (not assigned) but not zero'd. I think someone (me?) needs to do an audit on this and fix these variables.

@samsrabin
Copy link
Contributor Author

samsrabin commented May 23, 2024

Here are all the variables in the default list (from the LUH2 branch) that have data in the first output timestep but are all missing in the second:

  • FATES_BASALAREA_SZ
  • FATES_BURNFRAC_AP
  • FATES_CANOPYAREA_AP
  • FATES_CANOPYAREA_HT
  • FATES_CANOPYCROWNAREA_PF
  • FATES_CROWNAREA_CL
  • FATES_CROWNAREA_PF
  • FATES_DDBH_CANOPY_SZ
  • FATES_DDBH_USTORY_SZ
  • FATES_FIRE_INTENSITY_BURNFRAC_AP
  • FATES_FRAGMENTATION_SCALER_SL
  • FATES_FROOTC_SL
  • FATES_FUEL_AMOUNT_AP
  • FATES_FUEL_AMOUNT_FC
  • FATES_FUEL_BURNT_BURNFRAC_FC
  • FATES_FUEL_MOISTURE_FC
  • FATES_GPP_PF
  • FATES_GPP_SE_PF
  • FATES_LAI_CANOPY_SZ
  • FATES_LAI_USTORY_SZ
  • FATES_LEAFAREA_HT
  • FATES_LEAFC_PF
  • FATES_MORTALITY_AGESCEN_AC
  • FATES_MORTALITY_AGESCEN_SE_SZ
  • FATES_MORTALITY_AGESCEN_SZ
  • FATES_MORTALITY_BACKGROUND_SE_SZ
  • FATES_MORTALITY_BACKGROUND_SZ
  • FATES_MORTALITY_CANOPY_SE_SZ
  • FATES_MORTALITY_CANOPY_SZ
  • FATES_MORTALITY_CFLUX_PF
  • FATES_MORTALITY_CSTARV_CFLUX_PF
  • FATES_MORTALITY_CSTARV_SE_SZ
  • FATES_MORTALITY_CSTARV_SZ
  • FATES_MORTALITY_FIRE_SZ
  • FATES_MORTALITY_FREEZING_SE_SZ
  • FATES_MORTALITY_FREEZING_SZ
  • FATES_MORTALITY_HYDRAULIC_SE_SZ
  • FATES_MORTALITY_HYDRAULIC_SZ
  • FATES_MORTALITY_HYDRO_CFLUX_PF
  • FATES_MORTALITY_IMPACT_SZ
  • FATES_MORTALITY_LOGGING_SE_SZ
  • FATES_MORTALITY_LOGGING_SZ
  • FATES_MORTALITY_SENESCENCE_SE_SZ
  • FATES_MORTALITY_SENESCENCE_SZ
  • FATES_MORTALITY_TERMINATION_SZ
  • FATES_MORTALITY_USTORY_SZ
  • FATES_MORT_CSTARV_CONT_CFLUX_PF
  • FATES_NPLANT_AC
  • FATES_NPLANT_CANOPY_SZ
  • FATES_NPLANT_PF
  • FATES_NPLANT_SEC_PF
  • FATES_NPLANT_SZ
  • FATES_NPLANT_USTORY_SZ
  • FATES_NPP_PF
  • FATES_NPP_SE_PF
  • FATES_PATCHAREA_AP
  • FATES_PATCHAREA_LU
  • FATES_STOREC_PF
  • FATES_VEGC_ABOVEGROUND_SZ
  • FATES_VEGC_PF
  • FATES_VEGC_SE_PF

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug - software engineering history output Pertaining to FATES history output variables
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants