list_vpts_aloft() #579

PietrH · 2023-05-17T14:05:52Z

STATUS: READY FOR REVIEW

#553 (comment)

We have decided to build a function list_vpts_aloft() that returns a vector of urls that are known to exist, given the filtering parameters originally envisioned for download_vpts_alof()

list_vpts_aloft(
  date_min = NULL,
  date_max = NULL,
  radars = NULL,
  # directory = ".", This parameter is removed
  # overwrite = FALSE, This parameter is removed
  format = "csv" # also hdf5
  source = "baltrad", # also ecog-04003
)

Checking if a file exists can be done using the aws.s3 dependency via: aws.s3::get_bucket_df(bucket = "s3://aloft", prefix="baltrad/monthly", region = "eu-west-1", max = 2000) or much slower using httr: urls[!furrr::future_map_lgl(urls, ~httr::http_error(httr::HEAD(.x)))]

Dependencies

I'm using dplyr and magrittr pipes, so depend on replace data.table in regularize_vpts #574 to introduce them into the namespace.

Scope

Currently only format = "csv" is supported

Todo

merge master into download_vpts_aloft

codecov · 2023-05-22T08:21:25Z

Codecov Report

Merging #579 (53f764e) into master (a464125) will increase coverage by 0.88%.
The diff coverage is 100.00%.

❗ Current head 53f764e differs from pull request most recent head dce67e4. Consider uploading reports for the commit dce67e4 to get more accurate results

@@            Coverage Diff             @@
##           master     #579      +/-   ##
==========================================
+ Coverage   67.80%   68.68%   +0.88%     
==========================================
  Files          58       59       +1     
  Lines        3165     3254      +89     
==========================================
+ Hits         2146     2235      +89     
  Misses       1019     1019

Impacted Files	Coverage Δ
R/list_vpts_aloft.R	`100.00% <100.00%> (ø)`
R/zzz.R	`77.77% <100.00%> (+17.77%)`	⬆️

verbose is used differently (as write to stdout) in other functions

PietrH · 2023-05-23T09:00:47Z

Currently the function stops on an error if a radar station is provided that doesn't exist, however, when data for an existing radar station is simply missing, the function will offer a warning and continue to provide links for all other available data.

Do we want to stop on an error when no data is available for any of the provided radar stations?

used twice in `list_vpts_aloft()`

peterdesmet · 2023-05-23T09:58:31Z

when data for an existing radar station is simply missing, the function will offer a warning and continue to provide links for all other available data.

Good. Does it warn only once per radar (preferred) or also when time ranges are missing. I'm also fine with not having a warning for any missing data.

Do we want to stop on an error when no data is available for any of the provided radar stations?

No, just return an empty vector of paths

PietrH · 2023-05-23T10:04:14Z

I've decided to remove my purrr::map_chr() call with a very slightly slower (and more difficult to read) sapply to avoid having to state purrr as an Imports, even tough it's an implicit dependency already trough tidyr.

If this is overly defensive, feel free to revert d2c1e65. I vastly prefer using purrr over base apply functions, but we might get a CRAN warning if we have to many Imports.

PietrH · 2023-05-23T12:07:44Z

Good. Does it warn only once per radar (preferred) or also when time ranges are missing. I'm also fine with not having a warning for any missing data.

The warnings for radars and time ranges are independent, so it'll warn first if you are requesting a radar station that exists but has no data for your time range. Then it'll warn again if data was found for only a subset of the date range that was requested.

So you'll get two warnings at the maximum.

No, just return an empty vector of paths

Currently we are stopping on the error No data found for radars between {date_min} - {date_max}, I'll convert this to a warning and return an empty vector.

PietrH · 2023-05-23T13:59:05Z

Ready for review

iskandari

Looks good!

peterdesmet and others added 14 commits May 16, 2023 10:08

Create download_vpts_aloft.R

eacf5fc

use " not '

2d38e3e

limit to 80 cols

db50fa5

init list_vpts_aloft() + test

ec534e6

Merge pull request #576 from adokter/master

30b48c1

merge master into download_vpts_aloft

Update NAMESPACE

78bbfda

Check source

05bf9e8

Check output type

bb0ff4e

return vector of URLs

aa52213

comment out unused code, code sections

c1cfa8f

devtools::document()

c540842

add test headers

ae0c348

remove download_vpts_aloft()

da630d2

establish radars are within controlled list

40c25f6

PietrH added the enhancement label May 17, 2023

PietrH linked an issue May 17, 2023 that may be closed by this pull request

Create download_vpts_aloft() function #553

Closed

13 tasks

PietrH added 3 commits May 17, 2023 16:16

remove download_vpts_aloft() from NAMESPACE

a9b0459

add return documentation

91a7c41

remove download_vpts_aloft() documentation

787fa60

add tests regarding dates

d1d90fa

PietrH force-pushed the download_vpts_aloft branch from 401df4a to d1d90fa Compare May 22, 2023 08:38

PietrH added 5 commits May 22, 2023 10:39

explicit data masking to avoid R CMD CHECK warnings

71908ab

Merge branch 'master' into download_vpts_aloft

507ca60

handle missing dates

a0f021e

warn when no data for radar station within filter

7d88d9e

test format and source parameters

3d50ce9

bart1 mentioned this pull request May 22, 2023

make aws.s3 suggested (#583) #584

Merged

PietrH added 2 commits May 22, 2023 15:18

Check if aws.s3 is installed

b2d42c3

Handle cases where none or not all requested data was found

da4595a

PietrH added 4 commits May 23, 2023 10:42

rename verbose to show_warnings

ec95e60

verbose is used differently (as write to stdout) in other functions

only print warnings when show_warnings = TRUE

8234c2f

reword warning

2038e5c

style changes

a60850a

PietrH added 3 commits May 23, 2023 11:43

add helper to emulate stringr::str_extract()

7704f8e

used twice in `list_vpts_aloft()`

use extract_string helper: eliminate jsonlite Import

8fd9deb

devtools::document()

0ebf6ab

replace purrr::map_chr with sapply

d2c1e65

PietrH added 10 commits May 23, 2023 14:37

add test for extract_string() helper

6ffa3a2

return warning and empty vector on no data, styling

e90a2d2

remove purrr Import

6b91bf1

devtools::document()

2bc1dd2

don't use letters in test

2bbb5f8

Increase date_max till extra digit will be needed

85746e5

bump version number: dev referring to pull request

4319c16

add code section for final return() statement

fd3b1b5

avoiding sapply as per rOpenSci rec: not type safe

1fa5669

add list_vpts_aloft() to NEWS.md

dce67e4

PietrH marked this pull request as ready for review May 23, 2023 13:58

PietrH requested review from peterdesmet and adokter May 23, 2023 13:58

iskandari self-requested a review May 25, 2023 18:01

iskandari approved these changes May 25, 2023

View reviewed changes

iskandari merged commit 5261641 into master May 25, 2023
6 checks passed

peterdesmet deleted the download_vpts_aloft branch May 29, 2023 15:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

list_vpts_aloft() #579

list_vpts_aloft() #579

PietrH commented May 17, 2023 •

edited

codecov bot commented May 22, 2023 •

edited

PietrH commented May 23, 2023

peterdesmet commented May 23, 2023

PietrH commented May 23, 2023

PietrH commented May 23, 2023

PietrH commented May 23, 2023

iskandari left a comment

list_vpts_aloft() #579

list_vpts_aloft() #579

Conversation

PietrH commented May 17, 2023 • edited

STATUS: READY FOR REVIEW

Dependencies

Scope

Todo

codecov bot commented May 22, 2023 • edited

Codecov Report

PietrH commented May 23, 2023

peterdesmet commented May 23, 2023

PietrH commented May 23, 2023

PietrH commented May 23, 2023

PietrH commented May 23, 2023

iskandari left a comment

Choose a reason for hiding this comment

PietrH commented May 17, 2023 •

edited

codecov bot commented May 22, 2023 •

edited