updated modules/data.atmosphere #2775
Conversation
fixed tidyverse notes for the remaining variables in check_met_input.R for variables " is_required, cf_standard_name, test_passed, test_raw ".
fixed tidyverse notes for the remaining variables in download.NARR_site.R for variables " CF_name, year, data, month , startdate, hours ".
fixed tidyverse notes for the remaining variables "doy, rpot, avg.rpot, days" .
fixed tidyverse notes for the remaining variables " .attrs, canonical_units, description ".
deleted (relevant) lines starting <function name>: no visible binding for global variable <variable>.
There was a problem hiding this comment.
Most the suggested changes here are places I saw names in the data that reuse names defined elsewhere, and are therefore not showing up in the check warnings -- adding .data to these shouldn't change the code behavior, but disambiguating them should make it a lot easier to read the code!
Since most of these changes are additional edits rather than corrections, tagging @ashiklom to review them as the person most likely to know if I mangled a global that should stay global.
| @@ -43,14 +43,14 @@ download.NARR_site <- function(outfolder, | |||
| date_limits_chr <- strftime(range(narr_data$datetime), "%Y-%m-%d %H:%M:%S", tz = "UTC") | |||
|
|
|||
| narr_byyear <- narr_data %>% | |||
| dplyr::mutate(year = lubridate::year(datetime)) %>% | |||
| dplyr::group_by(year) %>% | |||
| dplyr::mutate(year = lubridate::.data$year(datetime)) %>% | |||
There was a problem hiding this comment.
| dplyr::mutate(year = lubridate::.data$year(datetime)) %>% | |
| dplyr::mutate(year = lubridate::year(datetime)) %>% |
| @@ -73,10 +73,10 @@ download.NARR_site <- function(outfolder, | |||
|
|
|||
| narr_proc <- result_full %>% | |||
| dplyr::mutate( | |||
| data_nc = purrr::map2(data, file, prepare_narr_year, lat = lat, lon = lon) | |||
| data_nc = purrr::map2(.data$data, file, prepare_narr_year, lat = lat, lon = lon) | |||
There was a problem hiding this comment.
| data_nc = purrr::map2(.data$data, file, prepare_narr_year, lat = lat, lon = lon) | |
| data_nc = purrr::map2(.data$data, .data$file, prepare_narr_year, lat = lat, lon = lon) |
There was a problem hiding this comment.
Hmm. @ashiklom is narr_proc needed at all? I don't see it used anywhere between here and the end of the function.
| @@ -225,7 +225,7 @@ get_NARR_thredds <- function(start_date, end_date, lat.in, lon.in, | |||
| PEcAn.logger::logger.info("Downloading in parallel") | |||
| flx_df$flx <- TRUE | |||
| sfc_df$flx <- FALSE | |||
| get_dfs <- dplyr::bind_rows(flx_df, sfc_df) | |||
| get_dfs <- dplyr::bind_rows(.data$flx_df, sfc_df) | |||
There was a problem hiding this comment.
| get_dfs <- dplyr::bind_rows(.data$flx_df, sfc_df) | |
| get_dfs <- dplyr::bind_rows(flx_df, sfc_df) |
Needed because (1) we want the flx_df defined on line 207, and (2) the arguments to bind_rows need to be whole dataframes rather than single columns.
| @@ -296,19 +296,19 @@ post_process <- function(dat) { | |||
| dat %>% | |||
| tidyr::unnest(data) %>% | |||
| dplyr::ungroup() %>% | |||
| dplyr::mutate(datetime = startdate + lubridate::dhours(dhours)) %>% | |||
| dplyr::select(-startdate, -dhours) %>% | |||
| dplyr::mutate(datetime = .data$startdate + lubridate::.data$dhours(.data$dhours)) %>% | |||
There was a problem hiding this comment.
| dplyr::mutate(datetime = .data$startdate + lubridate::.data$dhours(.data$dhours)) %>% | |
| dplyr::mutate(datetime = .data$startdate + lubridate::dhours(.data$dhours)) %>% |
| year = lubridate::.data$year(date), | ||
| month = lubridate::.data$month(date), | ||
| daygroup = daygroup(date, .data$flx) |
There was a problem hiding this comment.
| year = lubridate::.data$year(date), | |
| month = lubridate::.data$month(date), | |
| daygroup = daygroup(date, .data$flx) | |
| year = lubridate::.year(.data$date), | |
| month = lubridate::month(.data$date), | |
| daygroup = daygroup(.data$date, .data$flx) |
| unique(year), | ||
| unique(month), | ||
| unique(.data$year), | ||
| unique(.data$month), | ||
| unique(daygroup) |
There was a problem hiding this comment.
| unique(daygroup) | |
| unique(.data$daygroup) |
| unique(daygroup) | ||
| ) | ||
| ) %>% | ||
| dplyr::ungroup() %>% | ||
| dplyr::select(startdate, url) | ||
| dplyr::select(.data$startdate, url) |
There was a problem hiding this comment.
| dplyr::select(.data$startdate, url) | |
| dplyr::select(.data$startdate, .data$url) |
| @@ -382,12 +382,12 @@ get_narr_url <- function(url, xy, flx, pb = NULL) { | |||
| if (dhours[1] == 3) dhours <- dhours - 3 | |||
| narr_vars <- if (flx) narr_flx_vars else narr_sfc_vars | |||
| result <- purrr::pmap( | |||
| narr_vars %>% dplyr::select(variable = NARR_name, unit = units), | |||
| narr_vars %>% dplyr::select(variable = .data$NARR_name, unit = units), | |||
There was a problem hiding this comment.
| narr_vars %>% dplyr::select(variable = .data$NARR_name, unit = units), | |
| narr_vars %>% dplyr::select(variable = .data$NARR_name, unit = .data$units), |
| read_narr_var, | ||
| nc = nc, xy = xy, flx = flx, pb = pb | ||
| ) | ||
| names(result) <- narr_vars$CF_name | ||
| dplyr::bind_cols(dhours = dhours, result) | ||
| dplyr::bind_cols(dhours = .data$dhours, result) |
There was a problem hiding this comment.
| dplyr::bind_cols(dhours = .data$dhours, result) | |
| dplyr::bind_cols(dhours = dhours, result) |
| dplyr::mutate(hour = lubridate::hour(.data$time)) %>% | ||
| dplyr::mutate(doy = lubridate::yday(.data$time) + hour/(24/hr))%>% | ||
| dplyr::mutate(rpot = downscale_solar_geom(doy, as.vector(lon), as.vector(lat))) %>% # hourly sw flux calculated using solar geometry | ||
| dplyr::mutate(hour = lubridate::.data$hour(.data$time)) %>% |
There was a problem hiding this comment.
| dplyr::mutate(hour = lubridate::.data$hour(.data$time)) %>% | |
| dplyr::mutate(hour = lubridate::hour(.data$time)) %>% |
fixed the mistakes that were mentioned.
removed ' .data' pronoun from hour() function in line 83 .
|
/document |
mostly to trigger a CI run
| @@ -86,8 +86,8 @@ download.NARR_site <- function(outfolder, | |||
| #' @param file Full path to target file | |||
| #' @param lat_nc `ncdim` object for latitude | |||
| #' @param lon_nc `ncdim` object for longitude | |||
| #' @param verbose | |||
| #' @return List of NetCDF variables in data. Creates NetCDF file containing | |||
| #' @param verbose | |||
There was a problem hiding this comment.
| #' @param verbose | |
| #' @param verbose logical: ask`ncdf4` functions to be very chatty while they work? |
Documenting arguments is always good, but the reason we need this now is kind of 🙄: With the trailing space included, Roxygen was previousy complaining about "argument items with no description" and we ignored it in Rcheck_reference.log. Without the trailing space, Roxygen complains instead about "undocumented arguments" and causes a CI failure because the ignore-preexisting-errors script only knows how to skip exact test matches.
After fixing this we should delete the "with no description" from Rcheck_reference.log (currently lines 420-421).
| test_passed = !purrr::map_lgl(test_raw, inherits, "try-error"), | ||
| test_error_message = purrr::map_chr(test_raw, purrr::possibly(as.character, NA_character_)) |
There was a problem hiding this comment.
| test_passed = !purrr::map_lgl(test_raw, inherits, "try-error"), | |
| test_error_message = purrr::map_chr(test_raw, purrr::possibly(as.character, NA_character_)) | |
| test_passed = !purrr::map_lgl(.data$test_raw, inherits, "try-error"), | |
| test_error_message = purrr::map_chr(.data$test_raw, purrr::possibly(as.character, NA_character_)) |
TIL you can reuse previously defined columns inside a tibble definition! I had assumed this kind of transformation would need to be written df <- tibble::tibble(col_a = ...) %>% mutate(col_b = f(col_a, ...),...)
There was a problem hiding this comment.
Yup! IIRC, this is one of the key differences between tibble::tibble and data.frame -- it will not work in the latter.
| @@ -296,19 +296,19 @@ post_process <- function(dat) { | |||
| dat %>% | |||
| tidyr::unnest(data) %>% | |||
There was a problem hiding this comment.
| tidyr::unnest(data) %>% | |
| tidyr::unnest(.data$data) %>% |
|
/document |
|
@ashiklom @infotroph can you guys suggest to me the next issue I should start working on? |
|
@infotroph is there any other changes required here? |
|
@moki1202 Sorry for the delay here. The test failure in https://github.com/PecanProject/pecan/runs/2056814058#step:9:1353 ("column The next step is probably to run the tests locally and step through to verify what columns are actually present in the data at that step. |
|
@moki1202 Clarification: If you see how to fix the issue without stepping through the tests, go for it! That's just the next thing I'd do. |
|
@infotroph gave it a thorough check in the file download.NARR_site.R and didn't find any line where ' fix ' variable was not properly called using the ' .data$ ' pronoun....although, I did find out that I actually missed deleting the respective line regarding global variable error for ' flx ' variable in the Rcheck_reference.R . could this be a problem? |
Co-authored-by: Alexey Shiklomanov <alexey.shiklomanov@gmail.com>
|
@infotroph @ashiklom does this PR need any further work? |
|
@moki1202 Nope, we're just waiting for the build to turn green. The current failures are "unable to verify current time", which is apparently caused by R itself trying to consult a flaky third-party API. So definitely not your problem! |
fixed tidyverse notes for the remaining variables.