Submission: osmextract - Githubissues

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

Paste the full DESCRIPTION file inside a code block below:

Package: osmextract
Type: Package
Title: Download and Read OpenStreetMap Data Extracts
Version: 0.1.0
Authors@R: c(
    person("Andrea", "Gilardi", email = "a.gilardi5@campus.unimib.it", role=c("aut", "cre"),
      comment = c(ORCID = "0000-0002-9424-7439")),
    person("Robin", "Lovelace", role = c("aut"),
      comment = c(ORCID = "0000-0001-5679-6536")),
    person("Barry", "Rowlingson", role=c("ctb"),
      comment = c(ORCID = "0000-0002-8586-6625"))
    )
Description: This package is used to download, convert and read Open Street
    Map data extracts downloaded from several providers. 
License: GPL-3
Encoding: UTF-8
LazyData: true
RoxygenNote: 7.1.1
Roxygen: list(markdown = TRUE)
URL: https://github.com/itsleeds/osmextract
BugReports: https://github.com/itsleeds/osmextract/issues
Depends: R (>= 3.5.0)
Imports: 
    sf (>= 0.8.1),
    utils,
    tools
Suggests: 
    testthat,
    knitr,
    rmarkdown,
    covr
VignetteBuilder: knitr

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.):
- [x] data retrieval
- [x] data extraction
- [ ] data munging
- [ ] data deposition
- [ ] workflow automation
- [ ] version control
- [ ] citation management and bibliometrics
- [ ] scientific software wrappers
- [ ] field and lab reproducibility tools
- [ ] database software bindings
- [x] geospatial data
- [ ] text analysis
Explain how and why the package falls under these categories (briefly, 1-2 sentences):

osmextract is 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 .pbf files into modern geographic data structures (sf data frames).

Who is the target audience and what are scientific applications of this package?

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.

Are there other R packages that accomplish the same thing? If so, how does yours differ or meet our criteria for best-in-category?

osmdata provides 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.

(If applicable) Does your package comply with our guidance around Ethics, Data Privacy and Human Subjects Research?

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.

If you made a pre-submission enquiry, please paste the link to the corresponding issue, forum post, or other discussion, or @tag the editor you contacted.

Technical checks

Confirm each of the following by checking the box.

[x] I have read the guide for authors and rOpenSci packaging guide.

This package:

[x] does not violate the Terms of Service of any service it interacts with.
[x] has a CRAN and OSI accepted license.
[x] contains a README with instructions for installing the development version.
[x] includes documentation with examples for all functions, created with roxygen2.
[x] contains a vignette with examples of its essential functions and uses.
[x] has a test suite.
[x] has continuous integration, including reporting of test coverage using services such as Travis CI, Coveralls and/or CodeCov.

Publication options

[x] Do you intend for this package to go on CRAN?
[ ] Do you intend for this package to go on Bioconductor?
[ ] Do you wish to automatically submit to the Journal of Open Source Software? If so:

JOSS Options

- [ ] The package has an **obvious research application** according to [JOSS's definition](https://joss.readthedocs.io/en/latest/submitting.html#submission-requirements). - [ ] The package contains a `paper.md` matching [JOSS's requirements](https://joss.readthedocs.io/en/latest/submitting.html#what-should-my-paper-contain) with a high-level description in the package root or in `inst/`. - [ ] The package is deposited in a long-term repository with the DOI: - (*Do not submit your package separately to JOSS*)

[ ] Do you wish to submit an Applications Article about your package to Methods in Ecology and Evolution? If so:

MEE Options

- [ ] The package is novel and will be of interest to the broad readership of the journal. - [ ] The manuscript describing the package is no longer than 3000 words. - [ ] You intend to archive the code for the package in a long-term repository which meets the requirements of the journal (see [MEE's Policy on Publishing Code](http://besjournals.onlinelibrary.wiley.com/hub/journal/10.1111/(ISSN)2041-210X/journal-resources/policy-on-publishing-code.html)) - (*Scope: Do consider MEE's [Aims and Scope](http://besjournals.onlinelibrary.wiley.com/hub/journal/10.1111/(ISSN)2041-210X/aims-and-scope/read-full-aims-and-scope.html) for your manuscript. We make no guarantee that your manuscript will be within MEE scope.*) - (*Although not required, we strongly recommend having a full manuscript prepared when you submit here.*) - (*Please do not submit your package separately to Methods in Ecology and Evolution*)

Code of conduct

[x] I agree to abide by rOpenSci's Code of Conduct during the review process and in maintaining my package should it be accepted.

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:

[x] Fit: The package meets criteria for fit and overlap
[x] Automated tests: Package has a testing suite and is tested via Travis-CI or another CI service.
[x] License: The package has a CRAN or OSI accepted license
[x] Repository: The repository link resolves correctly
[ ] Archive (JOSS only, may be post-review): The repository DOI resolves correctly
[ ] Version (JOSS only, may be post-review): Does the release version given match the GitHub release (v1.0.0)?

Editor comments

Hello 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 vignettes

I'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 devtools::check().

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 halted

^{Created on 2020-09-29 by the reprex package (v0.3.0)}

Warning during testing

devtools::test() throws an error in:

test-read.R:3: error: oe_read: simplest examples work
#> cannot open URL ''

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:  1

^{Created on 2020-09-29 by the reprex package (v0.3.0)}

goodpractice output

There's a few minor issues flagged by goodpractice::gp()

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'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.

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 sessionInfo() and sf::sf_extSoftVersion()? @Robinlovelace suggested it may be a problem related to an old version of GDAL. Moreover, we recently changed the osmextract.Rmd vignette, could you please try running the checks using the most recent version of the package?

I think that the error in devtools::test has the same root cause as the error when installing the package.

and bunch of warnings

The warning messages are caused by a version of GDAL that does not support WKT, while the provider's data in osmextract was created using WKT. The problem is documented in the sf issue tracker. I added a note in the README:

https://github.com/ITSLeeds/osmextract/blob/1655ab4186c9a1402584234c3a9e808659940044/README.Rmd#L178-L187

and a warning message in .onLoad():

https://github.com/ITSLeeds/osmextract/blob/1655ab4186c9a1402584234c3a9e808659940044/R/zzz.R#L12-L25

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.

These warnings will tend to fade as OSs update and should disapper when installing newer version of PROJ and GDAL.

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 :smirk:):

R.Version()$version.string
#> [1] "R version 4.0.2 (2020-06-22)"
system2 ("gdalinfo", "--version", stdout = TRUE)
#> [1] "GDAL 3.0.4, released 2020/01/28"
system2 ("proj", "--version", stdout = TRUE)
#> [1] "Rel. 6.3.2, May 1st, 2020"

^{Created on 2020-10-01 by the reprex package (v0.3.0)}

(Tests just give the couple of warnings about st_centroid applied to lon-lat data.)

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-04

I've now updated GDAL to 3.1.2 using homebrew which also updated PROJ4 library to 7.1.1

==> Upgrading 1 outdated package:
gdal 2.4.4_2 -> 3.1.2_1

==> Installing gdal dependency: proj
==> Pouring proj-7.1.1.catalina.bottle.tar.gz

Just a note that I kept getting an error even after reinstalling sf from CRAN. It was still linking to an earlier version of proj.

> library(sf)
Linking to GEOS 3.7.2, GDAL 2.4.2, PROJ 5.2.0

FYI, I solved it by:

Set environment variable PKG_CONFIG_PATH to, in my case, /usr/local/lib/pkgconfig/, the path to to the pkgconfig/ directory containing a proj.pc file with details of the newest version, as suggested in this issue.
Reinstalled sf from GitHub with remotes::install_github("r-spatial/sf") (reinstalling from CRAN didn't solve the linking problem)

Which now works!

library("sf")
#> Linking to GEOS 3.8.1, GDAL 3.1.2, PROJ 7.1.1

But! I'm still having issues, see below:

tests

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       | downloadtrying 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

✓ |   6       | download [1.0 s]
✓ |   3       | find [0.6 s]
✓ |   1       | get
✓ |  26   3   | match [0.7 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
───────────────────────────────────────────────────────────────────────────────
✓ |   1       | providers
x |   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) 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.
✓ |   1       | update [0.6 s]
✓ |   7       | vectortranslate [0.3 s]

══ Results ════════════════════════════════════════════════════════════════════
Duration: 3.4 s

OK:       46
Failed:   1
Warnings: 3
Skipped:  1

checks

Also a couple more related errors in examples being thrown up by devtools::check()

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 halted

goodpractice

A 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
    strings

spellcheck

One valid (from what I can tell) spelling error thrown up by devtools::spell_check()

Antartica             geofabrik_zones.Rd:18

Coverage

I still can't get a coverage breakdown because I'm getting the same error when running covr::package_coverage()

Error: Failure in `/private/var/folders/8p/87cqdx2s34vfvcgh04l6z72w0000gn/T/RtmppGl6ac/R_LIBS61956a128da7/osmextract/osmextract-tests/testthat.Rout.fail` .osm.pbf' Content type 'application/octet-stream' length 40792 bytes (39 KB) ================================================== downloaded 39 KB ── 1. Error: oe_read: simplest examples work (@test-read.R#3) ──────────────── 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

Any ideas? It seems @mpadge didn't hit this issue 🤔

Hi @annakrystalli, and thank you very much again for your feedback.

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.

I just moved the warning message in the installation section, thanks!

goodpractice A few more minor issues thrown up by GP

We decided to adopt = as the assignment operator, so the first set of warning messages is somewhat "expected". I recently changed some of the "long lines of code" and, at the moment, the remaining ones should represent only long URL(s). I'm not sure about the UTF8 strings, but I think that's not a big deal.

spellcheck One valid (from what I can tell) spelling error thrown up by devtools::spell_check()

Fixed!

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:

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 + sf, so I have no idea what's going on 😢 The only thing that I may suggest is running (I read the following command here):

install.packages("sf", type = "mac.binary")
remotes::install_github("ITSLeeds/osmextract")

The first line of code should overwrite the previous version of sf, and install a "new" binary version with its own GEOS/GDAL/PROJ. The main drawback is that this "new" version of sf is not linked to the external GEOS/GDAL/PROJ (and this is the same behaviour that occurs on Windows, I think. @RobinLovelace please correct me here). I checked this approach with a friend of mine, and we installed the osmextract package without any problem.

Another question: do you face any problem when you install another R package that depends on sf?

EDIT: More info on the error in devtools::test(). I think that the error in oe_read() is caused by an erroneous installation of osmextract. The first two lines of that test are:

https://github.com/ITSLeeds/osmextract/blob/4fafa757fe0c7f701bd1b4833113346843ed6b1a/tests/testthat/test-read.R#L1-L2

Then, the object f is passed to oe_read(), which returns an error since (for some reason that I don't understand), f does not link any existing file and it's not a URL.

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:

FYI, I solved it by:

Set environment variable PKG_CONFIG_PATH to, in my case, /usr/local/lib/pkgconfig/, the path to to the pkgconfig/ directory containing a proj.pc file with details of the newest version, as suggested in this issue.

Reinstalled sf from GitHub with remotes::install_github("r-spatial/sf") (reinstalling from CRAN didn't solve the linking problem) Which now works!

But do you think the test failure and broken example still are an sf installation issue? The main issue in tests and checks seems to be:

  > 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.

But do you think the test failure and broken example still are an sf installation issue?

I think that the error is caused by some problems related to the installation of osmextract. The function oe_read is used to read-in a .pbf or .gpkg file, that can be specified using a file-path or a URL. It should fail only in two cases: 1) the input data points to a non-existing file or 2) the input data points to a non-existing URL.

The object my_pbf should represent the path to the its-example.osm.pbf file, which is one of the files included in the package (see https://github.com/ITSLeeds/osmextract/tree/master/inst). For example, I see:

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 devtools::test after deleting the "its-example.osm.pbf" file (which is clearly a patological example):

# 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:  1

^{Created 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 package_coverage() and everything seems good apart from zzz.R and update.R which are below ~75% coverage. Any particular reason for that?

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.

In the meantime, for completeness, I've now also ran package_coverage() and everything seems good apart from zzz.R and update.R which are below ~75% coverage. Any particular reason for that?

The file zzz.R includes only .onAttach() and the code lines that are not covered by tests are used to print an (hopefully) informative message in case GDAL or PROJ are "too old". I think that I cannot add any significative test in this case. Tomorrow I will add more tests for read.R and update.R!

One last thing @agila5 , could you please add the rOpenSci under review badge to your README?

[![](https://badges.ropensci.org/395_status.svg)](https://github.com/ropensci/software-review/issues/395)

I'll be back in touch when I have found both reviewers 👍

Reviewers: Due date:

Hi @annakrystalli and thank you very much for your assistance.

One last thing @agila5 , could you please add the rOpenSci under review badge to your README?

I added the badge to the README.Rmd but when I knit the document I see the following error:

Could not fetch https://badges.ropensci.org/395_status.svg
HttpExceptionRequest Request {
  host                 = "badges.ropensci.org"
  port                 = 443
  secure               = True
  requestHeaders       = []
  path                 = "/395_status.svg"
  queryString          = ""
  method               = "GET"
  proxy                = Nothing
  rawBody              = False
  redirectCount        = 10
  responseTimeout      = ResponseTimeoutDefault
  requestVersion       = HTTP/1.1
}
 (InternalException (HandshakeFailed (Error_Protocol ("certificate rejected: [NameMismatch \"badges.ropensci.org\"]",True,CertificateUnknown))))
Errore: pandoc document conversion failed with error 61
Esecuzione interrotta

I cannot see the badge on README.md but it's working on the website generated by pkgdown.

FIXED NOW

I've now found both reviewers! 🎉

Many thanks again @potterzot & @salvafern for agreeing to review.

Reviewers: @potterzot @salvafern Due date: 2020-12-21

Thank you very much, looking forward to both reviews!

Package Review

Briefly describe any working relationship you have (had) with the package authors. I have no working relationship with the authors.
[X] As the reviewer I confirm that there are no conflicts of interest for me to review this work (If you are unsure whether you are in conflict, please speak to your editor before starting your review).

Documentation

The package includes all the following forms of documentation:

[X] A statement of need clearly stating problems the software is designed to solve and its target audience in README
[X] Installation instructions: for the development version of package and any non-standard dependencies in README
[X] Vignette(s) demonstrating major functionality that runs successfully locally
[X] Function Documentation: for all exported functions in R help
[X] Examples for all exported functions in R Help that run successfully locally
[X] Community guidelines including contribution guidelines in the README or CONTRIBUTING, and DESCRIPTION with URL, BugReports and Maintainer (which may be autogenerated via Authors@R).

Since not submitting to JOSS, this is ignored:

For packages co-submitting to JOSS

[ ] The package has an obvious research application according to JOSS's definition

The package contains a paper.md matching JOSS's requirements with:

[ ] A short summary describing the high-level functionality of the software

[ ] Authors: A list of authors with their affiliations

[ ] A statement of need clearly stating problems the software is designed to solve and its target audience.

[ ] References: with DOIs for all those that have one (e.g. papers, datasets, software).

Functionality

[X] Installation: Installation succeeds as documented.
[X] Functionality: Any functional claims of the software been confirmed.
[X] Performance: Any performance claims of the software been confirmed.
[X] Automated tests: Unit tests cover essential functions of the package and a reasonable range of inputs and conditions. All tests pass on the local machine.
[X] Packaging guidelines: The package conforms to the rOpenSci packaging guidelines

Estimated hours spent reviewing: 4

[X] Should the author(s) deem it appropriate, I agree to be acknowledged as a package reviewer ("rev" role) in the package DESCRIPTION file.

Review Comments

First, 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

Please add a CONTRIBUTING.md or a section to the README that details how to contribute as per the community guidelines.
There are four warnings when I run devtools::test(), but looking at them I think that is expected behavior since the messages reflect tests of mispecified function arguments.

Larger Issues

oe_match(), oe_get(), and level

I ran into trouble when running oe_match. From the help message it seems like when a location matches to multiple levels, I could select the level I want. oe_match doesn't take a level parameter, but it also doesn't issue a warning about a level parameter. I ended up thinking it was fine and then also trying to use level = 2 in oe_get, which did issue a warning that was couched in other output, but then returned an error that seemed when I read it unrelated to the inclusion of the level. I suggest either changing the wording to make it clear that you cannot select a level or adding a level parameter.

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:

> yak <- tmaptools::geocode_OSM("Yakima, WA")$coords
> oe_match(yak, provider = "geofabrik", level = 2)

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
The input place was matched with multiple geographical areas with the same "level". Selecting the area whose centroid is closer 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] 162881219

Warning message:
In st_centroid.sfc(sf::st_geometry(matched_zones)) :
  st_centroid does not give correct centroids for longitude/latitude data

This selects the 'Washington State' geometry, but it would be nice if it selected the 'United States' geometry since I specified level = 2. Or return an error that says that level is not a valid parameter. The oe_get function provides a warning message along those lines, but it gets lost in the rest of the output and ultimately throws an error that is hard to relate back to the inclusion of the level argument:

> wa <- oe_get(yak, level = 2)

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
The input place was matched with multiple geographical areas with the same "level". Selecting the area whose centroid is closer to the input place
although coordinates are longitude/latitude, st_nearest_points assumes that they are planar
Warning: The following arguments are probably misspelled: level
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 `/tmp/RtmpumWNZh/geofabrik_washington-latest.gpkg' using driver `GPKG'
Error in st_sf(x, ..., agr = agr, sf_column_name = sf_column_name) : 
  no simple features geometry column present
In addition: Warning message:
In st_centroid.sfc(sf::st_geometry(matched_zones)) :

 Error in st_sf(x, ..., agr = agr, sf_column_name = sf_column_name) : 
  no simple features geometry column present

If I don't include the level = 2 argument in the oe_get function call it works perfectly fine:

> wa <- oe_get(yak)

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
The input place was matched with multiple geographical areas with the same "level". Selecting the area whose centroid is closer to the input place
although coordinates are longitude/latitude, st_nearest_points assumes that they are planar
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 `/tmp/RtmpumWNZh/geofabrik_washington-latest.gpkg' using driver `GPKG'
Simple feature collection with 1141221 features and 9 fields
geometry type:  LINESTRING
dimension:      XY
bbox:           xmin: -131.6931275 ymin: 41.9738173 xmax: -115.069417 ymax: 55.3538395
geographic CRS: WGS 84
Warning message:
In st_centroid.sfc(sf::st_geometry(matched_zones)) :
  st_centroid does not give correct centroids for longitude/latitude data

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 Review

Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide

Briefly describe any working relationship you have (had) with the package authors.
[x] As the reviewer I confirm that there are no conflicts of interest for me to review this work (If you are unsure whether you are in conflict, please speak to your editor before starting your review).

Documentation

The package includes all the following forms of documentation:

[x] A statement of need clearly stating problems the software is designed to solve and its target audience in README
[x] Installation instructions: for the development version of package and any non-standard dependencies in README
[x] Vignette(s) demonstrating major functionality that runs successfully locally
[x] Function Documentation: for all exported functions in R help
[x] Examples for all exported functions in R Help that run successfully locally
[ ] Community guidelines including contribution guidelines in the README or CONTRIBUTING, and DESCRIPTION with URL, BugReports and Maintainer (which may be autogenerated via Authors@R).

Not submitting to JOSS

For packages co-submitting to JOSS

[ ] The package has an obvious research application according to JOSS's definition

The package contains a paper.md matching JOSS's requirements with:

[ ] A short summary describing the high-level functionality of the software

[ ] Authors: A list of authors with their affiliations

[ ] A statement of need clearly stating problems the software is designed to solve and its target audience.

[ ] References: with DOIs for all those that have one (e.g. papers, datasets, software).

Functionality

[x] Installation: Installation succeeds as documented.
[x] Functionality: Any functional claims of the software been confirmed.
[x] Performance: Any performance claims of the software been confirmed.
[x] Automated tests: Unit tests cover essential functions of the package and a reasonable range of inputs and conditions. All tests pass on the local machine.
[ ] Packaging guidelines: The package conforms to the rOpenSci packaging guidelines

Estimated hours spent reviewing: 5

[x] Should the author(s) deem it appropriate, I agree to be acknowledged as a package reviewer ("rev" role) in the package DESCRIPTION file.

Review Comments

Hello @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.

Suggestion

On my first look at the README I thought that oe_get() could retrieve geometries from anywhere, and got a bit disappointed when I saw it was not the case. I think the oe_match() function deserves some attention in the README as it is really useful to find what geometries are available.

For example, the very first thing I tried was to look for a city I know, as I think most users will do:

oe_get("Ghent")
# Warning: The input place was matched with multiple geographical zones: Ghana - Kent. Selecting the first match.
# No exact matching found for place = Ghent. Best match is Ghana.
# Error: String distance between best match and the input place is 2, while the maximum threshold distance is equal to 1. You should increase the max_string_dist parameter, look for a closer match in the chosen provider database or consider using a different match_by variable.

But it wasn't until I read the osmextract vignette that I found it.

oe_match("Ghent", provider = "bbbike")
# The input place was matched with: Gent
# $url
# [1] "https://download.bbbike.org/osm/bbbike/Gent/Gent.osm.pbf"
# 
# $file_size
# [1] 13099190

In this case the issue was the provider, but I found it thanks to the oe_match() function.

Minor issues

Contributing guidelines: not included in the README or in a separated CONTRIBUTING file. Maybe add a reference to the providers vignette in the README.
Testing: 4 warnings (See my environment info below)

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

Code style: I ran styler:::style_active_pkg() and it changed 23 out of 25 files. I didn't compare the changes but maybe you want to have a look.

  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!

Citation: You could add your preferred citation (See https://devguide.ropensci.org/building.html#readme)
Continuous Integration: Apart of the code coverage, are you planning to add a CI system? (https://devguide.ropensci.org/ci.html#how-to-use-continuous-integration)

My environment

sessioninfo::platform_info()
# setting  value                       
# version  R version 4.0.2 (2020-06-22)
# os       Windows 10 x64              
# system   x86_64, mingw32             
# ui       RStudio                     
# language (EN)                        
# collate  English_United States.1252  
# ctype    English_United States.1252  
# tz       Europe/Paris                
# date     2020-12-16

Thanks for your timely review @salvafern !

Over to you now @agila!

Please note that there will be no editorial activity between Dec 19-Jan 3 #417. So no rush to respond over the holidays. Hope you get an opportunity to take a break and see you refreshed in the New Year!

Hi @salvafern, thank you very much for your review and your useful comments. @annakrystalli I will prepare an answer to both reviews in the next weeks!

Happy holidays to all the people that helped me during the review process ❄️ 🎆 🎉

Happy New Year all!

Just checking in @agila5 although as we had the holidays, its totally fine if you need more time to respond to the reviews.

Hi everyone and happy new year 🎉

I'm sorry that I didn't write any update. During the Christmas holidays, I focused on finishing the last parts of the dissertation (that must be submitted in a couple of days). We sketched a partial answer to the second review here, but I will prepare a more precise and detailed report for both reviews sometimes during the next week. Sorry for the delay.

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.

Please add a CONTRIBUTING.md or a section to the README that details how to contribute as per the community guidelines.

Contributing guidelines: not included in the README or in a separated CONTRIBUTING file. Maybe add a reference to the providers vignette in the README.

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.

There are four warnings when I run devtools::test(), but looking at them I think that is expected behavior since the messages reflect tests of mispecified function arguments.

Testing: 4 warnings (See my environment info below)

We decided to suppress some warning messages related to spatial operations in oe_match(), while the remaining ones are expected since they are related to tests regarding the warning messages.

I ran into trouble when running oe_match. From the help message, it seems like when a location matches to multiple levels, I could select the level I want. oe_match doesn't take a level parameter, but it also doesn't issue a warning about a level parameter. I ended up thinking it was fine and then also trying to use level = 2 in oe_get, which did issue a warning that was couched in other output, but then returned an error that seemed when I read it unrelated to the inclusion of the level. I suggest either changing the wording to make it clear that you cannot select a level or adding a level parameter.

We recently added a new level parameter to choose between multiple geographically nested areas. The default behaviour is the same as before, i.e. it selects the area associated with the highest level, which is linked with the smallest administrative unit:

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] 180538717

^{Created on 2021-01-18 by the reprex package (v0.3.0)}

The level argument can be used to choose a bigger administrative area:

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] 10776557870

^{Created 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)}

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.

The oe_get function provides a warning message along those lines, but it gets lost in the rest of the output and ultimately throws an error that is hard to relate back to the inclusion of the level argument:

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.

On my first look at the README I thought that oe_get() could retrieve geometries from anywhere, and got a bit disappointed when I saw it was not the case.

We changed the behaviour of oe_match() (and, as a consequence, oe_get()). Now, if the approximate string distance between the closest string match and the input place is greater than max_string_dist, then oe_match() will also check the other supported providers. For example:

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] 33969933

^{Created on 2021-01-18 by the reprex package (v0.3.0)}

We added a simple interface to Nominatim that enables oe_get() to 'geolocate' text strings that cannot be found in the providers. If the input place is not perfectly matched with any of the supported providers and match_by argument is equal to "name", then oe_match() will now, after https://github.com/ITSLeeds/osmextract/pull/155, use the Nominatim API to geolocate the input place and perform a spatial matching operation:

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] 423345383

^{Created on 2021-01-18 by the reprex package (v0.3.0)}

Code style: I ran styler:::style_active_pkg() and it changed 23 out of 25 files. I didn't compare the changes but maybe you want to have a look.

I think that this is simply caused by the fact that we use the = operator for assignment instead of <-.

Citation: You could add your preferred citation (See https://devguide.ropensci.org/building.html#readme)

We are working on a paper and we will add citation information as soon as possible.

Continuous Integration: Apart of the code coverage, are you planning to add a CI system?

We use Github Actions: https://github.com/ITSLeeds/osmextract/actions. Here you can browse the corresponding .yaml files.

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. 👍

@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 oe_match() and the Nominatim API work like a charm, thanks! I don't have further comments and I recommend this package.

Approved! 🎉📦✨

Thanks @agila5 for submitting and @potterzot & @salvafern for your reviews! 🤩

To-dos:

[ ] Transfer the repo to rOpenSci's "ropensci" GitHub organization under "Settings" in your repo. I have invited you to a team that should allow you to do so. You'll be made admin once you do.
[ ] Fix all links to the GitHub repo to point to the repo under the ropensci organization.
[ ] Delete your current code of conduct file if you had one since rOpenSci's default one will apply, see https://devguide.ropensci.org/collaboration.html#coc-file
[ ] If you already had a pkgdown website and are ok relying only on rOpenSci central docs building and branding,
- deactivate the automatic deployment you might have set up
- remove styling tweaks from your pkgdown config but keep that config file
- replace the whole current pkgdown website with a redirecting page
- replace your package docs URL with https://docs.ropensci.org/package_name
- In addition, in your DESCRIPTION file, include the docs link in the URL field alongside the link to the GitHub repository, e.g.: URL: https://docs.ropensci.org/foobar (website) https://github.com/ropensci/foobar
[ ] Fix any links in badges for CI and coverage to point to the ropensci URL. We no longer transfer Appveyor projects to ropensci Appveyor account so after transfer of your repo to rOpenSci's "ropensci" GitHub organization the badge should be [![AppVeyor Build Status](https://ci.appveyor.com/api/projects/status/github/ropensci/pkgname?branch=master&svg=true)](https://ci.appveyor.com/project/individualaccount/pkgname). If Appveyor does not pick up new commits after transfer, you might need to delete and re-create the Appveyor project. (Repo transfers are smoother with Travis CI and GitHub Actions)
[ ] We're starting to roll out software metadata files to all ropensci packages via the Codemeta initiative, see https://github.com/ropensci/codemetar/#codemetar for how to include it in your package, after installing the package - should be easy as running codemetar::write_codemeta() in the root of your package.

Should you want to acknowledge your reviewers in your package DESCRIPTION, you can do so by making them "rev"-type contributors in the Authors@R field (with their consent). More info on this here.

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 osmextract repo but it looks like I don't have enough permissions. I think that you create that repository several months ago so probably you are the only one who can transfer its ownership.

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.

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 osmextract and the Open Street Map ecosystem. As I said to Mark, I hope to write the first draft during the second half of January.

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!

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 NEWS.md file to keep track of these and future changes associated with new package versions.

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 DESCRIPTION file 😉

Ops, thanks, fixed that 😅

usethis::use_version()

Does it automatically ICYMI :tada:

ropensci / software-review

Submission: osmextract #395

Scope

Technical checks

Publication options

Code of conduct

Editor checks:

Editor comments

Problem with building vignettes

Warning during testing

goodpractice output

tests

checks

goodpractice

spellcheck

Coverage

Package Review

Documentation

For packages co-submitting to JOSS

Functionality

Review Comments

Small Issues

Larger Issues

Package Review

Documentation

For packages co-submitting to JOSS

Functionality

Review Comments

Suggestion

Minor issues

My environment