Skip to content

Conversation

@mattykim06
Copy link

…pools)

Description

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.

Copy link
Member

@mdietze mdietze left a comment

Choose a reason for hiding this comment

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

remove xlsx and Rdata files


# File that will initialize SIPNET. Takes in the UniqueID(polygon), the date (year and day),
# CLASS and SUBCLASS (ex. C2), and LAI
initialize_SIPNET <- function(UniqueID, Date, Event, CLASS_SUBCLASS, LAI) {
Copy link
Member

Choose a reason for hiding this comment

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

planting and harvest have different columns, so should be returned as different data frames

# Map CLASS and SUBCLASS
group_name <- get_crop_name(CLASS_SUBCLASS)

LAI = LAI * 10000
Copy link
Member

Choose a reason for hiding this comment

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

Not following this undocumented multiplication of a dimensionless variable by an arbitrary constant

if (Event == "planting") {
planting_params <- initialize_planting(group_name, LAI)

# Extract values with explicit NA handling
Copy link
Member

Choose a reason for hiding this comment

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

All this seems superfluous. Just put planting_params directly into the returned dataframe, no need to create all these temporary variables.

group_name <- get_crop_name(CLASS_SUBCLASS)


val_3446 <- get_fallback_value(group_name, 3446)
Copy link
Member

Choose a reason for hiding this comment

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

I'd recommend using meaningful variable names, not all these undocumented codes (e.g. 3446)

LAI = LAI * 10000

#val_601 <- get_fallback_value(group_name, trait_id = 601)
val_3441 <- get_fallback_value(group_name, trait_id = 3441)
Copy link
Member

Choose a reason for hiding this comment

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

separate the calculation of parameters from there use in calculating pools. Parameters should all be precalculated into a simple static lookup table based on group_name

C_coarseRoot.fraction <- 0.5
CN_root <- val_1055
CN_stem <- 70 #placeholder
SLA <- (val_3115 + val_3116 + val_3117) / 3
Copy link
Member

Choose a reason for hiding this comment

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

What's going on here? SLA is a single pre-calculated value, not the average of three things

coarseroot_nitrogen = N_coarseRoot
)

C_leaf <- planting_params[["leaf_carbon"]]
Copy link
Member

Choose a reason for hiding this comment

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

I'm not following this chunk of code and the one above it. The first seems to take the calculated pools and puts them into a list, the second seems to take those values out of the list and put them back into constants, in the end achieving nothing.

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.

2 participants