search

NGEET / fates

repository for the Functionally Assembled Terrestrial Ecosystem Simulator (FATES)
Other
105 stars 92 forks source link

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

Closed samsrabin closed 5 months ago

samsrabin commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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

rgknox commented 6 months ago

ok, looking into this now

samsrabin commented 6 months ago

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?
rgknox commented 6 months ago

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 commented 6 months ago

or is use_fates_ed_st3 = True ?

samsrabin commented 6 months ago

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

samsrabin commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

Yup, that also fixes the issue.

rgknox commented 6 months ago

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 commented 6 months ago

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: