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

Only write params with priors and/or data to config.xml #3125

Draft
wants to merge 12 commits into
base: develop
Choose a base branch
from

Conversation

Aariq
Copy link
Collaborator

@Aariq Aariq commented Mar 1, 2023

Description

Testing out what happens when PEcAn.ED2 doesn't write default constants to config.xml. An extreme fix for #3124

Motivation and Context

Would simplify PEcAn.ED2 code a lot and be less likely to accidentally overwrite constants in ways that break ED2

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 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.

@Aariq
Copy link
Collaborator Author

Aariq commented Mar 1, 2023

If this is a desirable PR, then I also would need to:

  • edit tests
  • remove history.* datasets
  • update code comments
  • update documentation
  • figure out if anything else uses the <revision> tag. If not, deprecate it in PEcAn.settings and pecan.xml documentation
  • [ ]

@mdietze
Copy link
Member

mdietze commented Mar 3, 2023

While I understand the disadvantages of the existing approach, and agree that things could be done to more easily keep the defaults more up-to-date, the decision to overwrite PFTs entirely was definitely done intentionally. If this approach is implemented I would strongly prefer that it be done as an optional flag rather than as the default behavior.

@Aariq
Copy link
Collaborator Author

Aariq commented Mar 3, 2023

I understand the motive behind overwriting all the parameters, but there's at least one case where this is having unintended consequences. For example at least b1Ht, b2Ht, and hgt_ref have different defaults (and meanings) depending on the choice of allometry equations set in ED2IN and in some cases these parameters don't have defaults, but are calculated from other parameters (rho) in ED2. There's more detail in #3124 as well as some ideas for alternative approaches to this PR. I'll keep this open as a draft for the time being if that's alright, as I'm using this version of PEcAn.ED2 to wrap up a project, at least until there is a better alternative.

@mdietze
Copy link
Member

mdietze commented Mar 4, 2023

Very few ED2 parameters are actually calculated based on other parameters. Most "calculations" are actually empirical correlations among traits that are hard coded into ED2 and in general those equations are old and ignore both uncertainties and correlations with other variables. The decision to avoid ED2's "calculations", and to handle these correlations within PEcAn, was very deliberate.

I understand that some parameters only make sense in combination with specific flags. If individuals updated the default flags without updating parameters that's a genuine error. If one wants to use different flags from the default they should probably set that as a different model version and adjust the parameters & priors accordingly.

@Aariq
Copy link
Collaborator Author

Aariq commented Mar 6, 2023

If individuals updated the default flags without updating parameters that's a genuine error.

This appears to be what happened. If ED version 2.2.0 or the github version of ED2 are used, the corresponding ED2IN file uses IALLOM = 3 and this bug (#3124) happens. The correct value of b1Ht and b2Ht for tropical PFTs can be calculated from rho with this function adapted from ED2 code:

iallom3 <- function(rho) {
  c14f15_ht_xx <- c(0.5709,-0.1007,0.6734)
  b1Ht = c14f15_ht_xx[1] + c14f15_ht_xx[2] * log(rho)
  b2Ht = c14f15_ht_xx[3]
  return(c(b1Ht = b1Ht, b2Ht = b2Ht))
}

I could try updating these two values for tropical PFTs in history.csv, or I could change IALLOM to the ED2 default (2) and change the values in history.csv to those defaults, OR I could revert IALLOM back to the legacy setting to match what's in history.csv. Any preference?

@Aariq
Copy link
Collaborator Author

Aariq commented Mar 6, 2023

If I wanted to use a different value for, say, IALLOM, how would I know which parameters in config.xml I would also have to change for it to work correctly? Would I have to look into the ED2 source code? Could I remove them from config.xml with PEcAn in some way, or would I have to write a script to edit all the config.xml files that PEcAn generates? I'm just trying to figure out how this is supposed to work.

@mdietze
Copy link
Member

mdietze commented Mar 7, 2023

If you want to PEcAn to use a specific non-default value for an ED2 parameter, rather than sample it from a distribution, then you put them in the <constants> section of the pft config https://pecanproject.github.io/pecan-documentation/develop/xml-core-config.html#xml-pft

As for how you know what different modes do, in theory that should be in the ED2 wiki, but I don't think people have been keeping it updated as they've made changes. So yeah, you mostly end up looking at the code.

@Aariq
Copy link
Collaborator Author

Aariq commented Sep 29, 2023

My work with PEcAn.ED2 is done, so you can close this if you'd like, but to be clear PEcAn.ED2 is currently broken without the changes in this PR (#3124), unless some other recent change has addressed this problem.

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

Successfully merging this pull request may close these issues.

PEcAn.ED2 defaults for allometry overwrite ED2 calculations and cause divide-by-zero errors
2 participants