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

Adding the feature of ERA5 download function using the CDS API #3447

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

Conversation

DongchenZ
Copy link
Contributor

@DongchenZ DongchenZ commented Feb 19, 2025

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.

Update bookdown documentation as needed

#'
#' @examples
#' @author Dongchen Zhang
ERA5_cds_annual_download <- function(years, months, days, times, area, variables, outdir, auto.create.key = T) {
Copy link
Member

Choose a reason for hiding this comment

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

recommend adjusting the function arguments to conform to existing patterns for met download functions. This would mean changing the name to download.ERA5_cds_annual and changing the order of the arguments to start with outfolder, start_date, end_date, then the spatial information, then any function-specific arguments, with required arguments coming before optional ones that have defaults.

Copy link
Member

Choose a reason for hiding this comment

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

Similar to my comment on the bounding box, I'd recommend that you internally construct the API's odd format for months, days, time, etc from the start and end times

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

#' @param months List: a list contains months to be downloaded (e.g., list("01", "02") to download files in Jan and Feb).
#' @param days List: a list contains days to be downloaded (e.g., list("01", "02") to download files in the first and second days).
#' @param times List: a list contains times to be downloaded (e.g., list('00:00','03:00') to download files for the times 12:00 and 3:00 am UTC).
#' @param area Character: a string contains the bounding box (formatted as "North/West/South/East") to be downloaded (e.g., "85/-179/7/-20").
Copy link
Member

Choose a reason for hiding this comment

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

This is a really odd format. Even if this is what the internal API expects, I think it would be a better design to take the bounding box as a vector of numbers and then format this string inside your function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

#' @examples
#' @author Dongchen Zhang
ERA5_cds_annual_download <- function(years, months, days, times, area, variables, outdir, auto.create.key = T) {
options(timeout=360000)
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 make this an optional argument to the function with a default, rather than hard coding.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

writeLines(c(
sprintf(
"url: %s",
getPass::getPass(msg = "Enter URL from the following link \n (https://cds.climate.copernicus.eu/api-how-to#install-the-cds-api-key):")
Copy link
Member

Choose a reason for hiding this comment

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

since getPass is in the suggests, you might add a step in the function that returns an informative error message if it isn't installed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

)
# convert grib to nc file.
nc.path <- gsub(".grib", ".nc", fname, fixed = T)
cmd <- paste("grib_to_netcdf", fname, "-o", nc.path)
Copy link
Member

Choose a reason for hiding this comment

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

what ensures that this command line utility exists in the user's system? it's not in the DESCRIPTION file, nor do you seem to provide any check that the command exists or instructions on how to install it. If you add a check, I'd do it early in the function -- would be a pain to have this function fail AFTER you've done the file download.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

# remove previous grib file.
unlink(fname)
}
return(nc.paths)
Copy link
Member

Choose a reason for hiding this comment

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

In keeping with PEcAn met patterns, I recommend returning a more complete results object. Look at other met download functions to see how these are structured. You may be using this function outside of the PEcAn met workflow, but that doesn't me we want to write it so that it can't be used within the PEcAn met workflow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

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