Submission: osmextract #395
Comments
|
Hello @agila5 and many thanks for your submission! Apologies for the slow response but many of us have been on holiday. I'll be your editor for this submission and will begin initial editor's checks now. Will be back in touch shortly! 👍 |
Editor checks:
Editor commentsHello again @agila5 ! 👋 I've now completed an initial pass of editor's checks. There's a few minor points but most importantly there is n error when building one of the vignettes (see below) and this is causing problems for me to fully complete the checks. If you could have a look and let me know when it's sorted, I can have another look. 👍 Problem with building vignettesI'm hitting a problem when building the vignettes from source locally, with same issue building from GitHub also: This is also causing problems when running pkg_dir <- "/Users/Anna/Documents/workflows/rOpenSci/editorials/osmextract"
devtools::install(pkg_dir, dependencies = T, build_vignettes = T)
#> checking for file ‘/Users/Anna/Documents/workflows/rOpenSci/editorials/osmextract/DESCRIPTION’ ... ✓ checking for file ‘/Users/Anna/Documents/workflows/rOpenSci/editorials/osmextract/DESCRIPTION’
#> ─ preparing ‘osmextract’:
#> checking DESCRIPTION meta-information ... ✓ checking DESCRIPTION meta-information
#> ─ installing the package to build vignettes
#> creating vignettes ... E creating vignettes (7.7s)
#> --- re-building ‘osmextract.Rmd’ using rmarkdown
#> trying URL 'https://github.com/ITSLeeds/osmextract/raw/master/inst/its-example.osm.pbf'
#> Content type 'application/octet-stream' length 40792 bytes (39 KB)
#> ==================================================
#> downloaded 39 KB
#>
#> Quitting from lines 566-573 (osmextract.Rmd)
#> Error: processing vignette 'osmextract.Rmd' failed with diagnostics:
#> NA value(s) in bounding box. Trying to plot empty geometries?
#> --- failed re-building ‘osmextract.Rmd’
#>
#> --- re-building ‘providers.Rmd’ using rmarkdown
#> --- finished re-building ‘providers.Rmd’
#>
#> SUMMARY: processing the following file failed:
#> ‘osmextract.Rmd’
#>
#> Error: Vignette re-building failed.
#> Execution halted
#>
#> Error in (function (command = NULL, args = character(), error_on_status = TRUE, : System command 'R' failed, exit status: 1, stdout + stderr:
#> E> * checking for file ‘/Users/Anna/Documents/workflows/rOpenSci/editorials/osmextract/DESCRIPTION’ ... OK
#> E> * preparing ‘osmextract’:
#> E> * checking DESCRIPTION meta-information ... OK
#> E> * installing the package to build vignettes
#> E> * creating vignettes ... ERROR
#> E> --- re-building ‘osmextract.Rmd’ using rmarkdown
#> E> trying URL 'https://github.com/ITSLeeds/osmextract/raw/master/inst/its-example.osm.pbf'
#> E> Content type 'application/octet-stream' length 40792 bytes (39 KB)
#> E> ==================================================
#> E> downloaded 39 KB
#> E>
#> E> Quitting from lines 566-573 (osmextract.Rmd)
#> E> Error: processing vignette 'osmextract.Rmd' failed with diagnostics:
#> E> NA value(s) in bounding box. Trying to plot empty geometries?
#> E> --- failed re-building ‘osmextract.Rmd’
#> E>
#> E> --- re-building ‘providers.Rmd’ using rmarkdown
#> E> --- finished re-building ‘providers.Rmd’
#> E>
#> E> SUMMARY: processing the following file failed:
#> E> ‘osmextract.Rmd’
#> E>
#> E> Error: Vignette re-building failed.
#> E> Execution haltedCreated on 2020-09-29 by the reprex package (v0.3.0) Warning during testing
and bunch of warnings pkg_dir <- "/Users/Anna/Documents/workflows/rOpenSci/editorials/osmextract"
devtools::test(pkg_dir)
#> Loading osmextract
#> Data (c) OpenStreetMap contributors, ODbL 1.0. https://www.openstreetmap.org/copyright.
#> Any product made from OpenStreetMap must cite OSM as the data source.
#> 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/
#> Testing osmextract
#> ✓ | OK F W S | Context
#> ⠏ | 0 | download⠙ | 1 1 | download✓ | 6 1 | download [0.6 s]
#> ─────────────────────────────────────────────────────────────────────────────────────────────────────
#> test-download.R:3: warning: oe_download: simplest examples work
#> st_crs<- : replacing crs does not reproject data; use st_transform for that
#> ─────────────────────────────────────────────────────────────────────────────────────────────────────
#> ⠏ | 0 | find⠼ | 1 4 | find✓ | 3 4 | find [0.6 s]
#> ─────────────────────────────────────────────────────────────────────────────────────────────────────
#> test-find.R:2: warning: oe_find: simplest example works
#> st_crs<- : replacing crs does not reproject data; use st_transform for that
#>
#> test-find.R:6: warning: oe_find: simplest example works
#> st_crs<- : replacing crs does not reproject data; use st_transform for that
#>
#> test-find.R:6: warning: oe_find: simplest example works
#> st_crs<- : replacing crs does not reproject data; use st_transform for that
#>
#> test-find.R:6: warning: oe_find: simplest example works
#> st_crs<- : replacing crs does not reproject data; use st_transform for that
#> ─────────────────────────────────────────────────────────────────────────────────────────────────────
#> ⠏ | 0 | get✓ | 1 1 | get
#> ─────────────────────────────────────────────────────────────────────────────────────────────────────
#> test-get.R:2: warning: oe_get: simplest examples work
#> st_crs<- : replacing crs does not reproject data; use st_transform for that
#> ─────────────────────────────────────────────────────────────────────────────────────────────────────
#> ⠏ | 0 | match⠏ | 4 6 | match⠸ | 8 16 | match⠴ | 14 22 | match✓ | 26 28 | match [0.4 s]
#> ─────────────────────────────────────────────────────────────────────────────────────────────────────
#> test-match.R:2: warning: oe_match: simplest examples work
#> st_crs<- : replacing crs does not reproject data; use st_transform for that
#>
#> test-match.R:2: warning: oe_match: simplest examples work
#> st_crs<- : replacing crs does not reproject data; use st_transform for that
#>
#> test-match.R:3: warning: oe_match: simplest examples work
#> st_crs<- : replacing crs does not reproject data; use st_transform for that
#>
#> test-match.R:3: warning: oe_match: simplest examples work
#> st_crs<- : replacing crs does not reproject data; use st_transform for that
#>
#> test-match.R:14: warning: oe_match: sfc_POINT objects
#> st_crs<- : replacing crs does not reproject data; use st_transform for that
#>
#> test-match.R:14: warning: oe_match: sfc_POINT objects
#> st_crs<- : replacing crs does not reproject data; use st_transform for that
#>
#> test-match.R:18: warning: oe_match: sfc_POINT objects
#> st_crs<- : replacing crs does not reproject data; use st_transform for that
#>
#> test-match.R:18: warning: oe_match: sfc_POINT objects
#> st_crs<- : replacing crs does not reproject data; use st_transform for that
#>
#> test-match.R:23: warning: oe_match: sfc_POINT objects
#> st_crs<- : replacing crs does not reproject data; use st_transform for that
#>
#> test-match.R:24: warning: oe_match: sfc_POINT objects
#> st_crs<- : replacing crs does not reproject data; use st_transform for that
#>
#> test-match.R:38: warning: oe_match: sfc_POINT objects
#> st_crs<- : replacing crs does not reproject data; use st_transform for that
#>
#> test-match.R:38: warning: oe_match: sfc_POINT objects
#> st_centroid does not give correct centroids for longitude/latitude data
#>
#> test-match.R:43: warning: oe_match: sfc_POINT objects
#> st_crs<- : replacing crs does not reproject data; use st_transform for that
#>
#> test-match.R:43: warning: oe_match: sfc_POINT objects
#> st_centroid does not give correct centroids for longitude/latitude data
#>
#> test-match.R:43: warning: oe_match: sfc_POINT objects
#> st_crs<- : replacing crs does not reproject data; use st_transform for that
#>
#> test-match.R:43: warning: oe_match: sfc_POINT objects
#> st_centroid does not give correct centroids for longitude/latitude data
#>
#> test-match.R:50: warning: oe_match: numeric input
#> st_crs<- : replacing crs does not reproject data; use st_transform for that
#>
#> test-match.R:50: warning: oe_match: numeric input
#> st_crs<- : replacing crs does not reproject data; use st_transform for that
#>
#> test-match.R:55: warning: oe_match: different providers, match_by or max_string dist args
#> st_crs<- : replacing crs does not reproject data; use st_transform for that
#>
#> test-match.R:56: warning: oe_match: different providers, match_by or max_string dist args
#> st_crs<- : replacing crs does not reproject data; use st_transform for that
#>
#> test-match.R:56: warning: oe_match: different providers, match_by or max_string dist args
#> st_crs<- : replacing crs does not reproject data; use st_transform for that
#>
#> test-match.R:58: warning: oe_match: different providers, match_by or max_string dist args
#> st_crs<- : replacing crs does not reproject data; use st_transform for that
#>
#> test-match.R:59: warning: oe_match: different providers, match_by or max_string dist args
#> st_crs<- : replacing crs does not reproject data; use st_transform for that
#>
#> test-match.R:59: warning: oe_match: different providers, match_by or max_string dist args
#> st_crs<- : replacing crs does not reproject data; use st_transform for that
#>
#> test-match.R:60: warning: oe_match: different providers, match_by or max_string dist args
#> st_crs<- : replacing crs does not reproject data; use st_transform for that
#>
#> test-match.R:84: warning: oe_check_pattern: simplest examples work
#> st_crs<- : replacing crs does not reproject data; use st_transform for that
#>
#> test-match.R:85: warning: oe_check_pattern: simplest examples work
#> st_crs<- : replacing crs does not reproject data; use st_transform for that
#>
#> test-match.R:86: warning: oe_check_pattern: simplest examples work
#> st_crs<- : replacing crs does not reproject data; use st_transform for that
#> ─────────────────────────────────────────────────────────────────────────────────────────────────────
#> ⠏ | 0 | providers✓ | 1 6 | providers
#> ─────────────────────────────────────────────────────────────────────────────────────────────────────
#> test-providers.R:2: warning: oe_providers works correctly
#> st_crs<- : replacing crs does not reproject data; use st_transform for that
#>
#> test-providers.R:2: warning: oe_providers works correctly
#> st_crs<- : replacing crs does not reproject data; use st_transform for that
#>
#> test-providers.R:2: warning: oe_providers works correctly
#> st_crs<- : replacing crs does not reproject data; use st_transform for that
#>
#> test-providers.R:2: warning: oe_providers works correctly
#> st_crs<- : replacing crs does not reproject data; use st_transform for that
#>
#> test-providers.R:2: warning: oe_providers works correctly
#> st_crs<- : replacing crs does not reproject data; use st_transform for that
#>
#> test-providers.R:2: warning: oe_providers works correctly
#> st_crs<- : replacing crs does not reproject data; use st_transform for that
#> ─────────────────────────────────────────────────────────────────────────────────────────────────────
#> ⠏ | 0 | readx | 0 1 1 1 | read
#> ─────────────────────────────────────────────────────────────────────────────────────────────────────
#> test-read.R:3: warning: oe_read: simplest examples work
#> URL '': status was 'URL using bad/illegal format or missing URL'
#>
#> test-read.R:3: error: oe_read: simplest examples work
#> cannot open URL ''
#> Backtrace:
#> 1. osmextract::oe_read(f) tests/testthat/test-read.R:3:2
#> 2. osmextract::oe_download(...) R/read.R:84:4
#> 3. utils::download.file(...) R/download.R:131:4
#>
#> test-read.R:23: skip: or_read: simplest example with a URL works
#> Reason: empty test
#> ─────────────────────────────────────────────────────────────────────────────────────────────────────
#> ⠏ | 0 | updateThe .gpkg files are going to be removed.
#> ⠹ | 1 2 | update✓ | 1 2 | update [0.4 s]
#> ─────────────────────────────────────────────────────────────────────────────────────────────────────
#> test-update.R:4: warning: oe_update(): simplest example works
#> st_crs<- : replacing crs does not reproject data; use st_transform for that
#>
#> test-update.R:10: warning: oe_update(): simplest example works
#> st_crs<- : replacing crs does not reproject data; use st_transform for that
#> ─────────────────────────────────────────────────────────────────────────────────────────────────────
#> ⠸ | 1 3 | update⠹ | 3 | vectortranslate✓ | 7 2 | vectortranslate [0.2 s]
#> ─────────────────────────────────────────────────────────────────────────────────────────────────────
#> test-vectortranslate.R:42: warning: oe_get_keys: simplest examples work
#> st_crs<- : replacing crs does not reproject data; use st_transform for that
#>
#> test-vectortranslate.R:49: warning: oe_get_keys: returns error with wrong inputs
#> st_crs<- : replacing crs does not reproject data; use st_transform for that
#> ─────────────────────────────────────────────────────────────────────────────────────────────────────
#>
#> ══ Results ══════════════════════════════════════════════════════════════════════════════════════════
#> Duration: 2.3 s
#>
#> OK: 45
#> Failed: 1
#> Warnings: 46
#> Skipped: 1Created on 2020-09-29 by the reprex package (v0.3.0) goodpractice outputThere's a few minor issues flagged by pkg_dir <- "/Users/Anna/Documents/workflows/rOpenSci/editorials/osmextract"
goodpractice::gp(pkg_dir)
#> Preparing: covr
#> Warning in MYPREPS[[prep]](state, quiet = quiet): Prep step for test coverage
#> failed.
#> Preparing: cyclocomp
#> Warning in MYPREPS[[prep]](state, quiet = quiet): Prep step for cyclomatic
#> complexity failed.
#> Preparing: description
#> Preparing: lintr
#> Preparing: namespace
#> Preparing: rcmdcheck
#> Warning in MYPREPS[[prep]](state, quiet = quiet): Prep step for rcmdcheck
#> failed.
#> ── GP osmextract ───────────────────────────────────────────────────────────────
#>
#> It is good practice to
#>
#> ✖ use '<-' for assignment instead of '='. '<-' is the standard, and R
#> users and developers are used it and it is easier to read your code
#> for them if you use '<-'.
#>
#> R/download.R:61:13
#> R/download.R:83:14
#> R/download.R:88:13
#> R/download.R:110:14
#> R/download.R:118:16
#> ... and 119 more lines
#>
#> ✖ avoid long code lines, it is bad for readability. Also, many people
#> prefer editor windows that are about 80 characters wide. Try make
#> your lines shorter than 80 characters
#>
#> R/download.R:40:1
#> R/find.R:23:1
#> R/find.R:57:1
#> R/get.R:9:1
#> R/get.R:72:1
#> ... and 17 more lines
#>
#> ────────────────────────────────────────────────────────────────────────────────Created on 2020-09-29 by the reprex package (v0.3.0) |
|
Hi @annakrystalli. First of all, thank you very much for testing our R package and helping us make it better.
I tried to reproduce the errors using my laptop (running Windows 10) and a server (running Ubuntu 16.04), but I couldn't replicate it. Could you please share the output of I think that the error in
The warning messages are caused by a version of GDAL that does not support WKT, while the provider's data in and a warning message in These warnings will tend to fade as OSs update and should disapper when installing newer version of PROJ and GDAL. Sorry for these problems, we'll try to fix them as soon as possible. |
For what it's worth, the package installs and tests clean on my (linux) system with absolute latest versions of everything (well, okay, not yet R4.0.3 😏): Created on 2020-10-01 by the reprex package (v0.3.0) (Tests just give the couple of warnings about |
|
Thanks for the prompt response @agila5 ! The message is useful as is the note in the README. However I would move that to the installation section and be explicit about the minimum versions of the GDAL and PROJ libraries required for full functionality. Now onto the fix I had a few issues with (so anticipate others might too!). Apologies for not sharing platform details earlier! Indeed I am on macos, here's full platform info: sessioninfo::platform_info()
#> setting value
#> version R version 3.6.2 (2019-12-12)
#> os macOS Catalina 10.15.6
#> system x86_64, darwin15.6.0
#> ui X11
#> language (EN)
#> collate en_GB.UTF-8
#> ctype en_GB.UTF-8
#> tz Europe/London
#> date 2020-10-04I've now updated Just a note that I kept getting an error even after reinstalling > library(sf)
Linking to GEOS 3.7.2, GDAL 2.4.2, PROJ 5.2.0FYI, I solved it by:
Which now works! library("sf")
#> Linking to GEOS 3.8.1, GDAL 3.1.2, PROJ 7.1.1But! I'm still having issues, see below: testschecksAlso a couple more related errors in examples being thrown up by E checking examples (2.8s)
Running examples in ‘osmextract-Ex.R’ failed
The error most likely occurred in:
> base::assign(".ptime", proc.time(), pos = "CheckExEnv")
> ### Name: oe_read
> ### Title: Read a '.pbf' or '.gpkg' object from file or URL
> ### Aliases: oe_read
>
> ### ** Examples
>
> # Read an existing .pbf file
> my_pbf = system.file("its-example.osm.pbf", package = "osmextract")
> oe_read(my_pbf, quiet = FALSE)
Error: The input file_path does not correspond to any existing file and it doesn't look like a URL.
Execution halted
✓ checking for unstated dependencies in ‘tests’ ...
─ checking tests ...
E Running ‘testthat.R’ (4s)
Running the tests in ‘tests/testthat.R’ failed.
Last 13 lines of output:
==================================================
downloaded 39 KB
── 1. Error: oe_read: simplest examples work (@test-read.R
The input file_path does not correspond to any existing file and it doesn't look like a URL.
Backtrace:
1. osmextract::oe_read(f)
The .gpkg files are going to be removed.
══ testthat results ═════════════════════════════════════
[ OK: 46 | SKIPPED: 1 | WARNINGS: 3 | FAILED: 1 ]
1. Error: oe_read: simplest examples work (@test-read.R#3)
Error: testthat unit tests failed
Execution halted
✓ checking for unstated dependencies in vignettes ...
✓ checking package vignettes in ‘inst/doc’ ...
✓ checking re-building of vignette outputs (10.6s)
✓ checking for detritus in the temp directory
See
‘/Users/Anna/Documents/workflows/rOpenSci/editorials/osmextract.Rcheck/00check.log’
for details.
── R CMD check results ─────────────────────────────────── osmextract 0.1.0 ────
Duration: 46.5s
> checking examples ... ERROR
Running examples in ‘osmextract-Ex.R’ failed
The error most likely occurred in:
> base::assign(".ptime", proc.time(), pos = "CheckExEnv")
> ### Name: oe_read
> ### Title: Read a '.pbf' or '.gpkg' object from file or URL
> ### Aliases: oe_read
>
> ### ** Examples
>
> # Read an existing .pbf file
> my_pbf = system.file("its-example.osm.pbf", package = "osmextract")
> oe_read(my_pbf, quiet = FALSE)
Error: The input file_path does not correspond to any existing file and it doesn't look like a URL.
Execution haltedgoodpracticeA few more minor issues thrown up by GP ── GP osmextract ──────────────────────────────────────────────────────────────
It is good practice to
✖ use '<-' for assignment instead of '='. '<-' is the
standard, and R users and developers are used it and it is easier
to read your code for them if you use '<-'.
R/download.R:61:13
R/download.R:83:14
R/download.R:88:13
R/download.R:110:14
R/download.R:119:16
... and 122 more lines
✖ avoid long code lines, it is bad for readability. Also,
many people prefer editor windows that are about 80 characters
wide. Try make your lines shorter than 80 characters
R/download.R:40:1
R/find.R:23:1
R/find.R:57:1
R/get.R:9:1
R/get.R:73:1
... and 22 more lines
✖ fix this R CMD check NOTE: Note: found 263 marked UTF-8
stringsspellcheckOne valid (from what I can tell) spelling error thrown up by CoverageI still can't get a coverage breakdown because I'm getting the same error when running Any ideas? It seems @mpadge didn't hit this issue 🤔 |
|
Hi @annakrystalli, and thank you very much again for your feedback.
I just moved the warning message in the installation section, thanks!
We decided to adopt
Fixed!
First of all, thanks for your useful feedback , I will add a link to the README pointing to this discussion as soon as we fix these problems. Unfortunately, I have very little experience with macOS + install.packages("sf", type = "mac.binary")
remotes::install_github("ITSLeeds/osmextract")The first line of code should overwrite the previous version of Another question: do you face any problem when you install another R package that depends on EDIT: More info on the error in Then, the object |
|
Hi @agila5 ! And thanks for your response! BTW I did manage to install! I just had to follow these steps to get everything to work properly:
But do you think the test failure and broken example still are an > my_pbf = system.file("its-example.osm.pbf", package = "osmextract")
> oe_read(my_pbf, quiet = FALSE)
Error: The input file_path does not correspond to any existing file and it doesn't look like a URL. |
I think that the error is caused by some problems related to the installation of The object list.files("C:/Users/Utente/Documents/R/win-library/3.6/osmextract/")
#> [1] "data" "DESCRIPTION" "help"
#> [4] "html" "INDEX" "its-example.osm.pbf"
#> [7] "Meta" "NAMESPACE" "osmconf.ini"
#> [10] "R"Could you compare that output with the analogous command on macOS? That example is clearly failing, and (I think) that should imply that there were some problems during the installation of the package. Could you try reinstalling it again? I can succesfully replicate the same behaviour only when I run # remove "old" osmextract
remove.packages("osmextract")
#> Removing package from 'C:/Users/Utente/Documents/R/win-library/3.6'
#> (as 'lib' is unspecified)
# check that it's removed
library("osmextract")
#> Error in library("osmextract"): there is no package called 'osmextract'
# download master branch
osmextract_master <- tempfile(fileext = ".zip")
download.file(
url = "https://github.com/ITSLeeds/osmextract/archive/master.zip",
destfile = osmextract_master
)
# unzip
unzip(osmextract_master, exdir = tempdir())
list.files(tempdir())
#> [1] "file2a2c23283a3e" "file2a2c3f246c27" "file2a2c54c36595.dll"
#> [4] "file2a2c7f3d50ae.zip" "osmextract-master"
# delete "its-example.osm.pbf" from osmextract-master
file.remove(file.path(tempdir(), "osmextract-master", "inst", "its-example.osm.pbf"))
#> [1] TRUE
# run tests
devtools::test(file.path(tempdir(), "osmextract-master"))
#> Loading osmextract
#> Data (c) OpenStreetMap contributors, ODbL 1.0. https://www.openstreetmap.org/copyright.
#> Any product made from OpenStreetMap must cite OSM as the data source.
#> 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/
#> Testing osmextract
#> v | OK F W S | Context
#> / | 0 | download- | 1 | downloadv | 6 | download [0.6 s]
#> / | 0 | find\ | 2 | findv | 3 | find [0.3 s]
#> / | 0 | getv | 1 | get
#> / | 0 | match/ | 4 | match\ | 6 | match- | 10 3 | matchv | 26 3 | match [0.5 s]
#> ----------------------------------------------------------------------------------------------------------------------------
#> test-match.R:38: warning: oe_match: sfc_POINT objects
#> st_centroid does not give correct centroids for longitude/latitude data
#>
#> test-match.R:43: warning: oe_match: sfc_POINT objects
#> st_centroid does not give correct centroids for longitude/latitude data
#>
#> test-match.R:43: warning: oe_match: sfc_POINT objects
#> st_centroid does not give correct centroids for longitude/latitude data
#> ----------------------------------------------------------------------------------------------------------------------------
#> / | 0 | providersv | 1 | providers
#> / | 0 | readx | 1 1 1 | read
#> ----------------------------------------------------------------------------------------------------------------------------
#> test-read.R:3: error: oe_read: simplest examples work
#> The input file_path does not correspond to any existing file and it doesn't look like a URL.
#> Backtrace:
#> 1. osmextract::oe_read(f) C:\Users\Utente\AppData\Local\Temp\Rtmp82J8ZK\osmextract-master/tests/testthat/test-read.R:3:2
#>
#> test-read.R:23: skip: or_read: simplest example with a URL works
#> Reason: empty test
#> ----------------------------------------------------------------------------------------------------------------------------
#> / | 0 | updateThe .gpkg files are going to be removed.
#> v | 1 | update
#> / | 0 | vectortranslate- | 1 | vectortranslate| | 3 | vectortranslatev | 7 | vectortranslate [0.4 s]
#>
#> == Results =================================================================================================================
#> Duration: 2.1 s
#>
#> OK: 46
#> Failed: 1
#> Warnings: 3
#> Skipped: 1Created on 2020-10-07 by the reprex package (v0.3.0) Sorry for all installation problems 😢 |
|
Hi @agila5 and many many apologies for the delay!! I completely missed your response 🤦♀️. I'll have a look at this today! |
|
Success! I recloned the original source code and everything seemed to work fine now. Really sorry for the hold up! I'll start looking for reviewers. In the meantime, for completeness, I've now also ran pkg_dir <- "/Users/Anna/Documents/workflows/rOpenSci/editorials/osmextract"
covr::package_coverage(pkg_dir)
#> osmextract Coverage: 82.76%
#> R/zzz.R: 58.82%
#> R/update.R: 68.25%
#> R/read.R: 74.19%
#> R/vectortranslate.R: 79.43%
#> R/download.R: 82.69%
#> R/find.R: 86.84%
#> R/match.R: 88.73%
#> R/utils.R: 91.67%
#> R/get.R: 100.00%
#> R/providers.R: 100.00%Created on 2020-11-02 by the reprex package (v0.3.0) |
|
Hi @annakrystalli and thanks for your comments. I'm really happy that now it's working fine 🎉 🎉 🎉 I will add a link in the README that points to this discussion since it could help new users.
The file |
|
One last thing @agila5 , could you please add the rOpenSci under review badge to your README? I'll be back in touch when I have found both reviewers 👍 Reviewers: |
This comment has been minimized.
This comment has been minimized.
|
I've now found both reviewers! 🎉 Many thanks again @potterzot & @salvafern for agreeing to review. Reviewers: @potterzot @salvafern |
|
Thank you very much, looking forward to both reviews! |
Package Review
DocumentationThe package includes all the following forms of documentation:
Since not submitting to JOSS, this is ignored:
Functionality
Estimated hours spent reviewing: 4
Review CommentsFirst, I wanted to say thank you for submitting this package. I was excited to review it since it pertains to potential future work I am hoping to do. In general, it's very well written and documented. It would be great to add some guidance on contributing. Other than that, there is really only one issue I came across. Small Issues
Larger Issuesoe_match(), oe_get(), and level I ran into trouble when running On a related issue, it's hard to know what "highest level" in the message means without reading. It might be clearer to say something like "smallest administrative unit" or "smallest geography", I'm not sure what the best term would be. Here's the code and output: This selects the 'Washington State' geometry, but it would be nice if it selected the 'United States' geometry since I specified If I don't include the |
|
Many thanks for your review @potterzot! @agila5, I would suggest refraining from making any changes in response to the review before the second review is in also (so @salvafern does not end up reviewing a moving target). When both are in, you can then make changes and respond to both at the same time. 😎 |
Package ReviewPlease check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide
DocumentationThe package includes all the following forms of documentation:
Not submitting to JOSS
Functionality
Estimated hours spent reviewing: 5
Review CommentsHello @agila5. I took some time to have a look at this, sorry for that. Let me say I enjoyed reviewing your package and that I could learn a couple things from it. I work daily with geospatial data and I hadn't seen before such a nice way to download, transform and cache data with R. I just got one suggestion and a few minor issues. SuggestionOn my first look at the README I thought that For example, the very first thing I tried was to look for a city I know, as I think most users will do: But it wasn't until I read the In this case the issue was the provider, but I found it thanks to the Minor issues
test-match.R:38: warning: oe_match: sfc_POINT objects
st_centroid does not give correct centroids for longitude/latitude data
test-match.R:43: warning: oe_match: sfc_POINT objects
st_centroid does not give correct centroids for longitude/latitude data
test-match.R:43: warning: oe_match: sfc_POINT objects
st_centroid does not give correct centroids for longitude/latitude data
test-read.R:46: warning: oe_read fails with misspelled arguments
The following arguments are probably misspelled: stringasfactor
styler:::style_active_pkg()
# Using style transformers `styler::tidyverse_style()`
# Styling 25 files:
# R/data.R The R.cache package needs to create a directory that will hold cache files. It is convenient to use ‘C:\Users\salvadorf\AppData\Local\R\R.cache’ because it follows the standard on your operating system and it remains also after restarting R. Do you wish to create the 'C:\Users\salvadorf\AppData\Local\R\R.cache' directory? If not, a temporary directory (C:\Users\SALVAD~1\AppData\Local\Temp\RtmpwneRPU/.Rcache) that is specific to this R session will be used. [Y/n]:
# Y
# i
# R/download.R i
# R/find.R i
# R/get.R i
# R/globals.R i
# R/match.R i
# R/providers.R i
# R/read.R i
# R/update.R i
# R/utils.R i
# R/vectortranslate.R i
# R/zzz.R i
# tests/testthat.R √
# tests/testthat/test-download.R i
# tests/testthat/test-find.R √
# tests/testthat/test-get.R i
# tests/testthat/test-match.R i
# tests/testthat/test-providers.R i
# tests/testthat/test-read.R i
# tests/testthat/test-update.R i
# tests/testthat/test-vectortranslate.R i
# data-raw/bbbike_zones.R i
# data-raw/geofabrik_zones.R i
# data-raw/openstreetmap_fr_zones.R i
# data-raw/test_zones.R i
# ----------------------------------------
# Status Count Legend
# √ 2 File unchanged.
# i 23 File changed.
# x 0 Styling threw an error.
# ----------------------------------------
# Please review the changes carefully!
My environment |
|
Thanks for the update @agila5 ! And not to worry. As I mentioned, technically the editorial process was on pause for the past two weeks. Good luck with your dissertation submission! |
|
Dear all, I just wanted to point out that tomorrow I plan to complete the last changes to the package, add tests during the weekend, and then answer all the comments during the first days of next week. |
|
Dear @potterzot and @salvafern, thank you very much for your reviews. You provided friendly comments and constructive criticisms that, I'm sure, helped us improve our package defining new functions and useful arguments. In the following parts of this response, I will provide several answers to all your comments.
We added the following text at the end of the README: We very much look forward to comments, questions and contributions. If you have any doubt, or if you want to suggest a new approach or add a new OSM provider, feel free to create a new issue in the issue tracker or a new pull request. We always try to build the most intuitive user interface and write the most informative error messages, but if you think that something is not clear and could have been explained better, please let us know.
We decided to suppress some warning messages related to spatial operations in
We recently added a new yak = c(-120.51084, 46.60156)
osmextract::oe_match(yak)
#> The input place was matched with multiple geographical areas.
#> Selecting the smallest administrative unit. Check ?oe_match for more details.
#> The input place was matched with multiple zones at the same level. Check ?oe_match for more details.
#> Selecting the area whose centroid is closest to the input place.
#> although coordinates are longitude/latitude, st_nearest_points assumes that they are planar
#> $url
#> [1] "https://download.geofabrik.de/north-america/us/washington-latest.osm.pbf"
#>
#> $file_size
#> [1] 180538717Created on 2021-01-18 by the reprex package (v0.3.0) The yak = c(-120.51084, 46.60156)
osmextract::oe_match(yak, level = 1)
#> The input place was matched with multiple geographical areas.
#> Selecting the desired level.
#> $url
#> [1] "https://download.geofabrik.de/north-america-latest.osm.pbf"
#>
#> $file_size
#> [1] 10776557870Created on 2021-01-18 by the reprex package (v0.3.0) It returns an error message if there is no area with the chosen level. yak = c(-120.51084, 46.60156)
osmextract::oe_match(yak, level = 3)
#> The input place was matched with multiple geographical areas.
#> Selecting the desired level.
#> Error in oe_match.sfc(place, provider = provider, quiet = quiet, ...): The input place does not intersect any area at the chosen level.Created on 2021-01-18 by the reprex package (v0.3.0)
We decided to suppress some warnings and simplify some messages. For example, the current output could be something like: #> The input place was matched with multiple geographical areas.
#> Selecting the smallest administrative unit. Check ?oe_match for more details.
#> The input place was matched with multiple zones at the same level. Check ?oe_match for more details.
#> Selecting the area whose centroid is closest to the input place.Do you think it's better now? We did not include many details in the message but just added a reference to the help pages.
We changed the behaviour of osmextract::oe_match("Madrid")
#> No exact match found for place = Madrid and provider = geofabrik. Best match is Mali.
#> Checking the other providers.
#> An exact string match was found using provider = bbbike.
#> $url
#> [1] "https://download.bbbike.org/osm/bbbike/Madrid/Madrid.osm.pbf"
#>
#> $file_size
#> [1] 33969933Created on 2021-01-18 by the reprex package (v0.3.0) We added a simple interface to Nominatim that enables osmextract::oe_match("Ghent")
#> Warning: The input place was matched with multiple geographical zones: Ghana -
#> Kent. Selecting the first match.
#> No exact match found for place = Ghent and provider = geofabrik. Best match is Ghana.
#> Checking the other providers.
#> No exact match found in any OSM provider data. Searching for the location online.
#> The input place was matched with multiple geographical areas.
#> Selecting the smallest administrative unit. Check ?oe_match for more details.
#> $url
#> [1] "https://download.geofabrik.de/europe/belgium-latest.osm.pbf"
#>
#> $file_size
#> [1] 423345383Created on 2021-01-18 by the reprex package (v0.3.0)
I think that this is simply caused by the fact that we use the
We are working on a paper and we will add citation information as soon as possible.
We use Github Actions: https://github.com/ITSLeeds/osmextract/actions. Here you can browse the corresponding |
|
Thanks for the thorough response @agila5! Over to you @potterzot & @salvafern. Please let us know if the response satisfies your comments or whether you feel there are still outstanding issues. 👍 |
annakrystalli
added
5/awaiting-reviewer(s)-response
and removed
4/review(s)-in-awaiting-changes
labels
|
@agila5 the new level parameter is great, thanks! @annakrystalli I have edited my review to reflect the added contribution language. I'm happy to recommend this package at this point. Please let me know if there's anything else you need from me. |
|
@salvafern would be great to get your input on the changes also. Many thanks. |
|
I overlooked this, sorry! @agila5 Both the new behaviour of |
|
Approved! 🎉📦✨ Thanks @agila5 for submitting and @potterzot & @salvafern for your reviews! 🤩 To-dos:
Should you want to acknowledge your reviewers in your package DESCRIPTION, you can do so by making them Welcome aboard! We'd love to host a post about your package - either a short introduction to it with an example for a technical audience or a longer post with some narrative about its development or something you learned, and an example of its use for a broader readership. If you are interested, consult the blog guide, and tag @stefaniebutland in your reply. She will get in touch about timing and can answer any questions. We've put together an online book with our best practice and tips, this chapter starts the 3d section that's about guidance for after onboarding. Please tell us what could be improved, the corresponding repo is here. |
|
Hi @annakrystalli and thank you very much for your assistance and for inviting me to join rOpenSci. @Robinlovelace I tried to transfer the ownership of Last but not least, @potterzot and @salvafern if you agree I would like to add you to package's description as reviewers. Thank you very much again for your comments. |
|
Ah ok, I will invite @Robinlovelace to the team also if that will make transfer easier. |
|
Hi @agila5, you can add me to the package's description as reviewer. Thank you! |
|
Hi guys, great to see the outcome of the review process, it definitely made the package better, very grateful for all the input. The fact that this now works is awesome (showing the road network in my hometown): library(osmextract)
#> Data (c) OpenStreetMap contributors, ODbL 1.0. https://www.openstreetmap.org/copyright.
#> Check the package website, itsleeds.github.io/osmextract for more details.
library(dplyr)
#>
#> Attaching package: 'dplyr'
#> The following objects are masked from 'package:stats':
#>
#> filter, lag
#> The following objects are masked from 'package:base':
#>
#> intersect, setdiff, setequal, union
osm_data_weobley = oe_get(place = "weobley", extra_tags = "maxspeed")
#> Warning: The input place was matched with multiple geographical zones: Isole -
#> Wales. Selecting the first match.
#> No exact match found for place = weobley and provider = geofabrik. Best match is Isole.
#> Checking the other providers.
#> No exact match found in any OSM provider data. Searching for the location online.
#> although coordinates are longitude/latitude, st_contains 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.
#> The chosen file was already detected in the download directory. Skip downloading.
#> The corresponding gpkg file was already detected. Skip vectortranslate operations.
#> Reading layer `lines' from data source `/mnt/57982e2a-2874-4246-a6fe-115c199bc6bd/data/osm/geofabrik_herefordshire-latest.gpkg' using driver `GPKG'
#> Simple feature collection with 37955 features and 10 fields
#> geometry type: LINESTRING
#> dimension: XY
#> bbox: xmin: -3.229032 ymin: 51.77741 xmax: -2.255137 ymax: 52.40357
#> geographic CRS: WGS 84
weobley_centre = osm_data_weobley %>%
filter(grepl(pattern = "Weobley", x = name)) %>%
sf::st_union() %>%
sf::st_centroid()
#> although coordinates are longitude/latitude, st_union assumes that they are planar
#> Warning in st_centroid.sfc(.): st_centroid does not give correct centroids for
#> longitude/latitude data
weobley_5km = stplanr::geo_buffer(weobley_centre, dist = 5000)
mapview::mapview(weobley_5km)osm_weobley = osm_data_weobley[weobley_5km, , op = sf::st_within]
#> although coordinates are longitude/latitude, st_within assumes that they are planar
plot(osm_weobley["highway"])Created on 2021-01-27 by the reprex package (v0.3.0) Will look to transfer the repo now. |
|
Job done:
|
|
Thanks! Now I will take care of the other points. |
|
Dear @annakrystalli, first of all thank you very much again for your support during the review process. A few hours ago @Robinlovelace transferred the repository to rOpenSci and I think I just completed the steps you mentioned in your previous comments (and I will add @potterzot as reviewer as soon as he agrees). In the next weeks we will also add a short introduction presenting the main functionalities in our package. |
|
@agila5 yes I would be happy to be added, thanks! |
|
@agila5 Congratulations on your osmextract passing review! I was happy to hear from Mark Padgham that you would like to write a post for the rOpenSci blog 😄. Timing is flexible; I understand you're very busy right now. When you're ready, please submit a draft post by pull request, following instructions in https://blogguide.ropensci.org/. Then my colleague @steffilazerte will review it and suggest a publication date. Cheers! |
|
Dear @stefaniebutland, thank you very much. I'm pleased to write a blog post for rOpenSci introducing
Ok! |
|
Thanks @agila5. If everything is now complete I will go ahead and close this issue now. I have also made you full admin of the repo again too. Look forward to the blogpost! One last thing. I just noticed that the package version is still the same as that submitted (0.1.0). Given the changes that have been made, the version should now be different. You might want to add a |
|
Dear @annakrystalli, thank you very much for your suggestions. We just released a new version and created the NEWS file. |
|
Fantastic! Don't forget to change the version on your |
|
Ops, thanks, fixed that 😅 |
Does it automatically ICYMI 🎉 |
Submitting Author: Andrea Gilardi (@agila5)
Repository: https://github.com/ITSLeeds/osmextract
Version submitted: 0.1.0
Editor: @annakrystalli
Reviewer 1: @potterzot
Reviewer 2: @salvafern
Archive: TBD
Version accepted: TBD
Scope
Please indicate which category or categories from our package fit policies this package falls under: (Please check an appropriate box below. If you are unsure, we suggest you make a pre-submission inquiry.):
Explain how and why the package falls under these categories (briefly, 1-2 sentences):
osmextractis for accessing and downloading OpenStreetMap data from a variety (current 2) of providers. OpenStreetMap has numerous scientific applications, including in transport, ecological analysis of human impacts on the environment, spatial statistics and land use change and the package provides a way of extracting the unstructure compressed.pbffiles into modern geographic data structures (sfdata frames).The target audience is researchers, practitioners and citizens who want to use OpenStreetMap data for scientific research, especially people who require large datasets (from city to continental scale) for their work.
osmdataprovides an R interface to the Overpass API, which is ideal for downloading small OSM datasets. However, the API is rate limited, making it hard to download large datasets.We believe the package complies with ethical guidelines in the Internet Research: Ethical Guidelines 3.0 document.
It makes it easier for researchers to access and make use of data that is already in the public domain, under the conditions of the adhering to the conditions of the OdBL.
Technical checks
Confirm each of the following by checking the box.
This package:
Publication options
JOSS Options
paper.mdmatching JOSS's requirements with a high-level description in the package root or ininst/.MEE Options
Code of conduct
The text was updated successfully, but these errors were encountered: