-
Notifications
You must be signed in to change notification settings - Fork 12
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
Add function for #154 #155
Conversation
Hi Robin! Thanks for the PR. I checked the review and I think it's quite positive 🎉. Anyway, I'm a little bit busy in the next days but I will work on osmextract during the weekend (or, anyway, as soon as possible). |
This is ready for review @agila5, no rush but it seems to work. Output is very chatty atm which is no bad thing I think, feedback welcome on idea and implementation:
|
Hi @Robinlovelace! Checking it right now. I was thinking of a slightly different workflow, but I will add some code for testing/discussing in a few hours. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Robinlovelace! I just pushed a commit for this PR. I slightly changed your approach in two ways: 1) the oe_search()
function now is in oe_match()
since it could be useful outside of oe_get()
and 2) oe_match()
now checks the other providers before using oe_serch()
. So, for example,
library(osmextract)
oe_match("Leeds")
#> No exact match found for place = Leeds and provider = geofabrik. Best match is Laos.
#> Checking the other providers.
#> I found an exact string match using provider = bbbike so I'm going to return that.
#> $url
#> [1] "https://download.bbbike.org/osm/bbbike/Leeds/Leeds.osm.pbf"
#>
#> $file_size
#> [1] 19376705
oe_match("Olginate")
#> No exact match found for place = Olginate and provider = geofabrik. Best match is Suriname.
#> Checking the other providers.
#> No exact match found in any OSM provider data. Searching for the location online.
#> although coordinates are longitude/latitude, st_intersects assumes that they are planar
#> The input place was matched with multiple geographical areas. Selecting the areas with the highest "level". See the help page associated to the chosen provider for an explanation of the meaning of the "level" field
#> $url
#> [1] "https://download.geofabrik.de/europe/italy/nord-ovest-latest.osm.pbf"
#>
#> $file_size
#> [1] 416306623
Created on 2020-12-28 by the reprex package (v0.3.0)
Now I need to review examples and vignettes and add some tests
Great thinking on including it in |
Another thought: worth using |
I would like to keep the two operations in two distinct functions since if you want just to download PBF/GPKG data for a given provider, then you can run |
Sometimes next week I will finish re-reading the vignette and then I think it will be ready for review/merge! |
I think this is almost ready to go. Let me know if you want me to add something to the vignette. I've already used the new functionality in my work, useful for sure! |
Hi Robin! Thanks for the comment. If you can test this PR and find any problem, please report it. I plan to finish the PR on Monday 11/01 and then start with #160 |
Thanks @agila5, sounds good to me! |
Hi @Robinlovelace, I think it's ready for review. |
Hi @Robinlovelace, sorry to bother you. Can you check the PR today or tomorrow morning? Tomorrow (after lunch) I would like to complete the other PRs and write the answer for rOpenSci review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
@@ -36,10 +36,10 @@ | |||
#' @export | |||
#' | |||
#' @examples | |||
#' its_match = oe_match("ITS Leeds", provider = "test") | |||
#' its_match = oe_match("ITS Leeds", provider = "test", quiet = TRUE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
#' west_yorkshire = oe_get("West Yorkshire", quiet = FALSE) | ||
#' # Match with place name | ||
#' oe_get("Milan") # Warning: the .pbf file is 400MB |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great example!
|
||
# 2. Check the other providers and, if there is an exact match, just return | ||
# the matched value from the other provider: | ||
other_providers = setdiff(oe_available_providers(), provider) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -367,7 +427,7 @@ oe_match_pattern = function( | |||
|
|||
# Then we extract only the elements of the match_by_column that match the | |||
# input pattern. | |||
match_ID = grep(pattern, match_by_column) | |||
match_ID = grep(pattern, match_by_column, ignore.case = TRUE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
R/zzz.R
Outdated
"Geofabrik data are taken from https://download.geofabrik.de/", | ||
"For usage details of bbbike data see https://download.bbbike.org/osm/", | ||
"OpenStreetMap_fr data are taken from http://download.openstreetmap.fr/", | ||
"Check the package website for more details.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One suggestion: Add the link to the website.
No description provided.