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

list_vpts_aloft() #579

Merged
merged 66 commits into from
May 25, 2023
Merged

list_vpts_aloft() #579

merged 66 commits into from
May 25, 2023

Conversation

PietrH
Copy link
Collaborator

@PietrH PietrH commented May 17, 2023

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

Scope

Currently only format = "csv" is supported

Todo

  • unit tests
  • roxygen documentation
  • handle missing dates: return all data
  • bump version number
  • aws.s3 to Suggests
  • purrr & jsonlite to Suggests
  • consider using lubridate in filter instead of strings
  • consider removing hard coded dates when not provided
  • fix pkgdown bug: _pkgdown.yml
  • Stop when a radar is missing
  • Add verbose argument to silence warnings
  • warn twice when both dates and radars are missing
  • Update NEWS.md
  • remove jsonlite dependency
  • consider extract_string helper: used twice
  • remove purrr dependency
  • emtpy vector and warning on no data

@PietrH PietrH linked an issue May 17, 2023 that may be closed by this pull request
13 tasks
@codecov
Copy link

codecov bot commented May 22, 2023

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%) ⬆️

@PietrH
Copy link
Collaborator Author

PietrH commented May 23, 2023

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?

@peterdesmet
Copy link
Collaborator

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
Copy link
Collaborator Author

PietrH commented May 23, 2023

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
Copy link
Collaborator Author

PietrH commented May 23, 2023

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 PietrH marked this pull request as ready for review May 23, 2023 13:58
@PietrH
Copy link
Collaborator Author

PietrH commented May 23, 2023

Ready for review

@iskandari iskandari self-requested a review May 25, 2023 18:01
Copy link
Collaborator

@iskandari iskandari left a comment

Choose a reason for hiding this comment

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

Looks good!

@iskandari iskandari merged commit 5261641 into master May 25, 2023
@peterdesmet peterdesmet deleted the download_vpts_aloft branch May 29, 2023 15:39
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.

Create download_vpts_aloft() function
3 participants