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

Integrate matt changes #3460

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

Conversation

yinghaoSunn
Copy link
Contributor

Description

Merge Matt's modification on LPJGUESS into the develop branch.
Make sure there's no conflict with the current version.

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.

MagicForrest and others added 3 commits June 27, 2019 16:26
…biomass. Note that the problem with negative cmass_heart is not yet resolved.
Merge remote-tracking branch 'matt_repo/LPJGUESS_SDA' into integrate_matt_changes

# Conflicts:
#	models/lpjguess/R/update.state.LPJGUESS.R
#	models/lpjguess/man/update_state_LPJGUESS.Rd
#	models/lpjguess/man/write.config.LPJGUESS.Rd
##'
##'
##' @keywords internal
##' @return the scaled 'individual' (the initial nested list with update values)
##' @author Matthew Forrest
adjust.biomass.LPJGUESS <- function(individual, rel.change, sla, wooddens, lifeform, k_latosa, k_allom2, k_allom3){
adjust.biomass.LPJGUESS <- function(individual, biomass.inc, sla, wooddens, lifeform, k_latosa, k_allom2, k_allom3, trace = TRUE){
Copy link
Member

Choose a reason for hiding this comment

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

Changes in function arguments should be accompanied by changes in @param definitions


#paramsins <- readLines(file.path(rundir, "params.ins"), n = -1)
#npatches <- as.numeric(gsub(".*([0-9]+).*$", "\\1", paramsins[grepl("npatch", paramsins, fixed = TRUE)]))
npatches <- 5
Copy link
Member

Choose a reason for hiding this comment

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

I suspect this particular change was for debugging purposes and wasn't meant to be hard coded as 5

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are some similar cases such as calculateGridcellVariablePerPFT function, where the input min.diam=5. But I think it's reasonable to exclude trees with a DBH below 5 cm.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll revert this to the original and see if it throws an error.

##' Calculate Above Ground Woody Biomass

##' @keywords internal
AbvGrndWood <- function(individual, include.debt = TRUE){
Copy link
Member

Choose a reason for hiding this comment

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

would be good to document internal functions too

##' Calculate Above Ground Woody Biomass

##' @keywords internal
TotalCarbon <- function(individual, include.debt = TRUE){
Copy link
Member

Choose a reason for hiding this comment

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

document

…ndWood`, `TotalCarbon`;

2. Fix the hard code `npatches <- 5` in `read_state.R`
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.

3 participants