get_historical crashing when no BOM Stations returned from call in .get_ncc

paulr-bv commented 4 years ago

get_historical() is currently failing. Symptom is:

library(bomrang)
# Get data for a station
df <- get_historical(stationid = "023000", type = "max")

Error returned is:

> df <- get_historical(stationid = "023000", type = "max") 
Error in x[i] : only 0's may be mixed with negative subscripts

I did a quick investigation and I think it is failing in .get_ncc()

It seems that there is a bug on the BOM site today where a call for rain stations is returning zero (0) stations. The call is:

http://www.bom.gov.au/climate/data/lists_by_element/alphaAUS_136.txt

It currently returns:

Bureau of Meteorology product IDCJMC0014. Produced: 20 Aug 2020 Australian stations measuring rainfall

0 stations

(c) Copyright Commonwealth of Australia 2020, Bureau of Meteorology (ABN 92 637 533 532) Please note Copyright, Disclaimer and Privacy Notice, accessible at http://www.bom.gov.au/other/copyright.shtml

The call in .get_ncc for this 0 stations have the following for ncc:

> ncc
# A tibble: 2 x 11
  site                         name          lat                         lon   start_month start_year end_month end_year years percent AWS  
  <chr>                        <chr>         <chr>                       <chr> <chr>       <chr>      <chr>     <chr>    <chr> <chr>   <chr>
1 (c) Copyright Commonwealth … Bureau of Me… (ABN 92 637 533 532)        NA    NA          NA         NA        NA       NA    NA      NA   
2 Please note Copyright, Disc… Notice, acce… <http://www.bom.gov.au/oth… NA    NA          NA         NA        NA       NA    NA      NA   
>

In the .get_ncc function, this seems to be causing the error on line 264 of the current get_historical.R file (the second line in this quote below):

    # trim the end of the rows off that have extra info that's not in columns
    nrows <- nrow(ncc) - 7
    ncc <- ncc[1:nrows, ]

This call appears to be trying to exclude the copyright blocks at the bottom of the "data".

The code appears to assume there will be 7 lines.
Note, in the current call for http://www.bom.gov.au/climate/data/lists_by_element/alphaAUS_122.txt there only appears to be three lines of copyright info, not 7 - this might be excluding data?

Suggestion:

It seems that this function needs to gracefully handle the failure of these BOM http calls not returning any data.
If updating the code, handle the copyright block maybe more dynamically too.

One option is to update / replace current lines 262 - 273 with something like this:

# # trim the end of the rows off that have extra info that's not in columns
# nrows <- nrow(ncc) - 7
# ncc <- ncc[1:nrows, ]

# Remove the copyright block from the bottom.
# Currently the `lon` column is NA across all 4 data sets for copyright block
ncc <-
  ncc %>% 
  dplyr::filter(!is.na(lon))

if (nrow(ncc) > 0) {
  # unite month and year, convert to a date and add ncc_obs_code
  ncc <-
    ncc %>%
    tidyr::unite(start, start_month, start_year, sep = "-") %>%
    tidyr::unite(end, end_month, end_year, sep = "-") %>%
    dplyr::mutate(start = lubridate::dmy(paste0("01-", start))) %>%
    dplyr::mutate(end = lubridate::dmy(paste0("01-", end))) %>%
    dplyr::mutate(ncc_obs_code = ncc_obs_code)
}

adamhsparks commented 4 years ago

Possibly related to https://github.com/ropensci/bomrang/issues/119

paulr-bv commented 4 years ago

Yes. Apologies for not seeing #199 :-( Possible solution included here though incase the BOM issue remains

adamhsparks commented 4 years ago

@rensa, how far did you get with your bug fix?

jimjam-slam commented 4 years ago

I didn't get to it today, unfortunately! However, we do need it fixed for our work, so I'll 100% be taking a swing at it on Monday morning.

adamhsparks commented 4 years ago

No worries, I'm otherwise occupied at the moment.

jimjam-slam commented 4 years ago

I've submitted a PR with a fix to the immediate problem, although I've had it drop a warning if any of the station lists are empty (which may be confusing if you, say, request temperature and it says the rain list is missing). It can be merged as-is (although CI will fail, as it's expecting output from get_historical_weather), or we could rework the function to only request the station list for the requested data type.

adamhsparks commented 4 years ago

I've incorporated @rensa's PR and updated tests to handle this. If rain is requested, this will automatically error out, all others still receive the warning message about lack of availability for rain data.

I need to update the tests for the min, max and solar to reflect the warning message if any of them goes offline as rain currently has.

I've another new function I'm working on to fetch sub-daily timescales using stationaRy::get_met(). Once I've incorporated that, I'll get a new release pushed to CRAN. In the meantime, you should be able to install and use the latest version now from GitHub using:

if (!require("remotes")) {
  install.packages("remotes", repos = "http://cran.rstudio.com/")
  library("remotes")
}

install_github("ropensci/bomrang", build_vignettes = TRUE)

paulr-bv commented 4 years ago

Adam,

Thanks for the heads up on this being available to install from GitHub. I have just done that and it mostly works. ☺

The issue is:

get_historical() is not supported or the backward compatibility isn’t there.

Here is an example (my bolding obviously):

Canberra_mintemps <- get_historical(latlon = c(-35.2809, 149.1300),

type = "min") Error in get_historical(latlon = c(-35.2809, 149.13), type = "min") : could not find function "get_historical"

In the new version (v 0.7.0.9000):

get_historical() does not exist anymore.
get_historical_weather() exists instead.

I refer to the current documentation on CRAN for v0.7.0 which shows “get_historical()” should be there (and code snippet above is from the example)

https://cran.r-project.org/web/packages/bomrang/vignettes/bomrang.html#using-get_historical

I tweaked my code to point to get_historical_weather() and it worked fine, with the rain station data not being available.

Thanks again for all your work on this.

Cheers, Paul.

From: "Adam H. Sparks" notifications@github.com Reply to: ropensci/bomrang reply@reply.github.com Date: Sunday, 30 August 2020 at 8:10 pm To: ropensci/bomrang bomrang@noreply.github.com Cc: Paul Robertson paulr@beovista.com.au, Author author@noreply.github.com Subject: Re: [ropensci/bomrang] get_historical crashing when no BOM Stations returned from call in .get_ncc (#120)

I've incorporated @rensahttps://github.com/rensa's PR and updated tests to handle this. If rain is requested, this will automatically error out, all others still receive the warning message about lack of availability for rain data.

I need to update the tests for the min, max and solar to reflect the warning message if any of them goes offline as rain currently has.

I've another new function I'm working on to fetch sub-daily timescales using stationaRy::get_met(). Once I've incorporated that, I'll get a new release pushed to CRAN. In the meantime, you should be able to install and use the latest version now from GitHub using:

if (!require("remotes")) {

install.packages("remotes", repos = "http://cran.rstudio.com/")

library("remotes")

}

install_github("ropensci/bomrang", build_vignettes = TRUE)

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/ropensci/bomrang/issues/120#issuecomment-683401639, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AG7QKKZQXPW5SX5C4Y2PMGLSDIQPPANCNFSM4QG5M4DA.

ropensci-archive / bomrang

get_historical crashing when no BOM Stations returned from call in .get_ncc #120