Skip to content

Enhance ERA5 Download Function: Direct NetCDF, Flexible Parameters, and Improved Docs - #3547

Merged
dlebauer merged 34 commits into
PecanProject:developfrom
divine7022:era5-cdsapi-tweaks
Aug 18, 2025
Merged

Enhance ERA5 Download Function: Direct NetCDF, Flexible Parameters, and Improved Docs#3547
dlebauer merged 34 commits into
PecanProject:developfrom
divine7022:era5-cdsapi-tweaks

Conversation

@divine7022

@divine7022 divine7022 commented Jun 14, 2025

Copy link
Copy Markdown
Member

Description

This PR refactors the download.ERA5_cds function to:

  • Download ERA5 data directly in netcdf format (removes grib_to_netcdf conversion dependency).
  • Parameters are now passed in the correct vector format to ensure proper API requests.
  • Added support for flexible selection of product type, dataset, and time steps (no longer hardcoded).
  • Fixed the extent argument to ensure correct spatial subsetting as required by the API.
  • Provided clearer, beginner-friendly documentation and example usage.
  • Removed the Python dependency( cdsapi via reticulate ) and migrating to the native R ecmwfr package
  • added a unit tests for the extraction function(test.download.ERA5_cds.R)

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.

@dlebauer
dlebauer requested review from DongchenZ and mdietze June 17, 2025 21:31
Comment thread modules/data.atmosphere/R/ERA5_download.R Outdated

@dlebauer dlebauer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks great - thanks for these tests.

I'm running one of the examples, and it has been running for ~20 minutes. How long do you expect the examples to take? This seems long for an example / proof of concept.

I've made a few minor suggestions, including one that generally applies to your work - please add your name to the author field when you have made non-trivial changes to functions!

Comment thread modules/data.atmosphere/R/ERA5_download.R Outdated
Comment thread modules/data.atmosphere/R/ERA5_download.R Outdated
Comment thread modules/data.atmosphere/R/ERA5_download.R Outdated
#' @export
#'
#' @importFrom purrr %>%
#' @author Dongchen Zhang

@dlebauer dlebauer Aug 4, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
#' @author Dongchen Zhang
#' @author Dongchen Zhang, Akash BV

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

👍

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

As commented elsewhere, we use pkg::fn notation which makes @importFrom not necessary

Comment thread modules/data.atmosphere/R/ERA5_download.R
Comment thread modules/data.atmosphere/R/ERA5_download.R Outdated
Comment thread modules/data.atmosphere/R/ERA5_download.R Outdated

@dlebauer dlebauer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hi @divine7022 thanks again for this major update to the ERA5 code, it looks great - and is very close!

Mostly there are just a few minor changes to implement, including a number of suggested changes that you 👍🏻 but were not committed. These are now marked 'outdated' but not 'resolved'.

#' @export
#'
#' @importFrom purrr %>%
#' @author Dongchen Zhang

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

As commented elsewhere, we use pkg::fn notation which makes @importFrom not necessary

Comment thread modules/data.atmosphere/R/ERA5_download.R Outdated
Comment on lines +66 to +68
download.ERA5_cds <- function(outfolder, start_date, end_date,
extent, variables, time = NULL, dataset = "reanalysis-era5-single-levels",
product_type = "ensemble_members", user = NULL, key = NULL, timeout = 36000) {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

this is the current function signature;
auto.create.key = TRUE argument is outdated -- it was used when the function relied on Python via reticulate.
Now that we’ve switched to using ecmwfr.

@infotroph infotroph Aug 14, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Are user and key optional or required here? A NULL default typically means more or less "the function will figure this out if not provided", which would be aligned with the previous getnetrc approach. But on a quick skim below I see an error if user or key are null and didn't see anything that would fill them in before that -- if they're now required arguments that's fine but might as well remove the default value to make that clearer.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

thanks! @infotroph now removed NULL defaults for user/key parameters

@divine7022

Copy link
Copy Markdown
Member Author

As commented elsewhere, we use pkg::fn notation which makes @importFrom not necessary

@dlebauer thanks! for the catch - now uses base R pipe |> instead of %>%
i hope all the suggestions have been addressed.

@infotroph

Copy link
Copy Markdown
Member

now uses base R pipe |> instead of %>%

Thank you for that change! Note that this means the package now requires a version of R new enough to contain the base pipe, but I don't think the package checks detect this yet. Can you please add Depends: R (>= 4.1) at line 22 of the DESCRIPTION file (between the Description: and Imports: sections)?

Comment thread modules/data.atmosphere/NEWS.md Outdated
@divine7022

Copy link
Copy Markdown
Member Author

thanks @infotroph now Depends: R (>= 4.1.0) added to DESCRIPTION

Comment thread modules/data.atmosphere/R/ERA5_download.R Outdated
Comment thread modules/data.atmosphere/R/ERA5_download.R
Comment thread modules/data.atmosphere/DESCRIPTION Outdated

@dlebauer dlebauer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for making these changes - looks like all requested changes have been made.

Apologies some of my earlier requests were based outdated code.

@dlebauer
dlebauer enabled auto-merge August 18, 2025 19:46
@dlebauer
dlebauer added this pull request to the merge queue Aug 18, 2025
@divine7022

Copy link
Copy Markdown
Member Author

Thanks for approving @dlebauer

Merged via the queue into PecanProject:develop with commit 42bbe7a Aug 18, 2025
18 of 25 checks passed
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.

3 participants