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

Updates of fates model #3233

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

Updates of fates model #3233

wants to merge 25 commits into from

Conversation

Hhh-hyc
Copy link

@Hhh-hyc Hhh-hyc commented Nov 9, 2023

Description

Changes made in this pull request:

  • docker-compose.yml: added a package named CTSM-FATES for further integration (on going).
  • DESCRIPTION: replaced the old name 'fates' with 'CTSM-FATES' for clarity, the version of CTSM-FATES will be added later.
  • met2model.FATES.R: was updated to generate three monthly files, including precipitation, solar radiation and temperature+other variables, respectively. The shared variables, such as longitude, latitude, EDGEW, EDGEN, etc., were pre-defined and then written into .nc files along with unique variables for each type of files, e.g, precipitation flux for precipitation file, surface downwelling flux for solar file, etc. The file structures are in line with the default GSWP3 forcing files used by CTSM-FATES. The generated input files have been tested with ctsm5.1.dev108 and FATES sci.1.58.1_api.24.1.0 for FATES (with NUOPC coupler). The function has been tested with stand-alone R. Testing within PEcAN is still needed.
  • model2netcdf.FATES.R: both PFT level output and grid level output variables from CLM-FATES have been considered. Other cohort/patch level outputs such as different age and size groups, however, have not been implemented yet. Any suggestions on this are welcome. Currently, The function has been tested with stand-alone R assuming default monthly output from CLM-FATES. Extending the function to deal with daily output from CLM-FATES and testing the function within PEcAn is still needed.
  • template.job: tentative changes were made. is stil a work ongoing.

Motivation and Context

Integrate the newest version of CTSM-FATES into PEcAn. CTSM-FATES is assumed to be run in a docker or openshift container. The work is relevant to the issue on PEcAN #1008 and the issue on FATES: NGEET/fates#364

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.

@istfer istfer self-assigned this Nov 9, 2023
models/fates/R/met2model.FATES.R Outdated Show resolved Hide resolved
models/fates/R/met2model.FATES.R Outdated Show resolved Hide resolved
models/fates/R/met2model.FATES.R Outdated Show resolved Hide resolved
models/fates/R/met2model.FATES.R Outdated Show resolved Hide resolved
models/fates/R/met2model.FATES.R Outdated Show resolved Hide resolved
models/fates/R/met2model.FATES.R Outdated Show resolved Hide resolved
models/fates/R/met2model.FATES.R Outdated Show resolved Hide resolved
models/fates/R/model2netcdf.FATES.R Outdated Show resolved Hide resolved
models/fates/R/model2netcdf.FATES.R Outdated Show resolved Hide resolved
models/fates/R/model2netcdf.FATES.R Outdated Show resolved Hide resolved
models/fates/DESCRIPTION Outdated Show resolved Hide resolved
@istfer
Copy link
Contributor

istfer commented Nov 9, 2023

@Hhh-hyc Yucong congratulations on your first PEcAn PR, thank you for contributing. Let's iterate with the suggested revisions.

models/fates/R/met2model.FATES.R Outdated Show resolved Hide resolved
models/fates/R/met2model.FATES.R Outdated Show resolved Hide resolved
out$dat[[i]] <- dat.new
}
library(ncdf4.helpers) # dims names
model2netcdf.FATES <- function(outdir, sitelat, sitelon, pfts) {
Copy link
Member

Choose a reason for hiding this comment

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

  1. Arguments to the function don't match the arguments in the documentation.
  2. Not sure why these new additional arguments are needed. Generally you want to convert ALL of the variables you can from a model run into PEcAn standard, and then select any subset of info you need when you read the PEcAn netCDF files (e.g. specific PFTs, variables, and locations)

@mdietze
Copy link
Member

mdietze commented Feb 25, 2024

@Hhh-hyc wanted to ping you about finishing up this PR, which has been in the queue for a while

@mdietze
Copy link
Member

mdietze commented Apr 1, 2024

 R check of models/fates reports the following new problems. Please fix these and resubmit:
  checking dependencies in R code ... WARNING
  '::' or ':::' import not declared from: ‘ncdf4.helpers’
  checking for code/documentation mismatches ... WARNING
  Codoc mismatches from documentation object 'met2model.FATES':
  met2model.FATES
    Code: function(in.path, in.prefix, outfolder, start_date, end_date,
                   lst = 0, lat, lon, overwrite, verbose)
    Docs: function(in.path, in.prefix, outfolder, start_date, end_date,
                   lst = 0, lat, lon, overwrite = FALSE, verbose = FALSE,
                   ...)
    Argument names in docs not in code:
      ...
    Mismatches in argument default values:
      Name: 'overwrite' Code:  Docs: FALSE
      Name: 'verbose' Code:  Docs: FALSE
  
  Codoc mismatches from documentation object 'model2netcdf.FATES':
  model2netcdf.FATES
    Code: function(outdir, sitelat, sitelon, pfts)
    Docs: function(outdir)
    Argument names in code not in docs:
      sitelat sitelon pfts
  
  checking R code for possible problems ... NOTE
  model2netcdf.FATES : var_update: no visible global function definition
    for ‘show’
  Undefined global functions or variables:
    show

@mdietze
Copy link
Member

mdietze commented May 16, 2024

@Hhh-hyc checking in again on this PR which seems to require fairly minor changes to pass the GH Action checks. Is this something you'll be able to get back to soon?

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.

None yet

4 participants