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

Handle missing database$bety$write + nonmissing site$site.pft #3419

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

infotroph
Copy link
Member

Description

Found a few more places where it was ambiguous whether a NULL settings$database$bety$write should be treated as TRUE or FALSE, and set them to treat it as FALSE. For consistency I also changed the one place I saw that was treating missing as TRUE -- If I was following previous discussion in #3398, we've at least implicitly decided missing should mean do not write.

Also: write.configs.SIPNET() now respects run$site$site.pft and uses it preferentially to run$inputs$pft.site. If anyone feels strongly I'd be open to switching the priority so pft.site wins if both are defined -- I picked this way because it felt like it was keeping effects best localized to the place they were set.

Motivation and Context

Review Time Estimate

  • Immediately
  • Within one week
  • When possible

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My change requires a change to the documentation.
  • My name is in the list of CITATION.cff
  • I agree that PEcAn Project may distribute my contribution under any or all of
    • the same license as the existing code,
    • and/or the BSD 3-clause license.
  • I have updated the CHANGELOG.md.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

site_pft <- utils::read.csv(settings$run$inputs$pft.site$path)
site.pft.name <- site_pft$pft[site_pft$site == settings$run$site$id]
}
if (site.pft.name != "boreal.coniferous") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is just a reorganization of existing code, but the hard-coded PFTs and Months here is dangerous because it represents hidden assumptions that are now getting applied at continental scales without the users being aware of these assumptions.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How much additional work would it be to remove these? Do existing PFT definitions contain information we could use to pick the right LAI here without the hard-coding?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we should be able to figure out the phenological type of a PFT based on it's parameters. I'm not entirely sure, off the top of my head, how the evergreen PFTs have been parameterized as evergreen, but for example I would expect fracLeafFall to be a number near 1 for deciduous species and a number <0.5 for evergreen (evergreens often do have a seasonal leaf phenology, but they might drop more like 25% of their LAI in the winter). Similarly, rather than hard coding dates we should look at the MODIS phenology file (if provided) or the leaf on/off parameters to know whether (for any DOY) we should initialize a specific site in a leaf on or off state

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You know, what's the chance we could push this logic all the way into a different workflow? If the fundamental question is "how many leaves should we assume we're starting from", why not answer it when creating the IC pool files rather than when reading them in to Sipnet?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get that argument, but I think there are a number of clear disadvantages. First, routine use of IC files in PEcAn runs is very new and I don't think we can assume it's universally adopted. Second, this would be a HUGE increase in the required temporal precision of initialization. Right now it's not uncommon to grab the "nearest available" IC data, which for vegetation and C pools can sometimes be up to a decade shifted in time. Pushing phenology into the IC means having to make IC files that are specific to an individual start date, and rebuilding the files if you change the start date at all. To me, adding in a simple check to see if, for the specified start date, whether the site should be in a leaf-on or leaf-off state isn't a huge burdern for write.configs that pays off with greater IC flexibility.

@mdietze
Copy link
Member

mdietze commented Jan 23, 2025

I think I prefer the idea that if the pft.site file is defined it wins. Otherwise you could end up in a situation in multisite runs where some sites use pft.site and others use site$pft, which doesn't seem like the intended behavior and would make it hard to extract exactly what PFT was run at each site. In general, pft.site shouldn't be used for single-site runs, but is awfully handy for multisite runs.

@github-actions github-actions bot added the Tests label Jan 23, 2025
@infotroph
Copy link
Member Author

@mdietze I've switched the logic to make pft.site win when both are defined. Thoughts on whether to merge now or tackle the hard-coded PFT/month logic?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants