-
Notifications
You must be signed in to change notification settings - Fork 12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Get the package peer reviewed #35
Comments
As advocated by @agila5 here #32 (comment) |
Draft of submission: Submitting Author: Name (@agila5)
Scope
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.
We believe the package complies with ethical guidelines in the Internet Research: Ethical Guidelines 3.0 document.
Technical checksConfirm each of the following by checking the box.
This package:
Publication options
JOSS Options
MEE Options
Code of conduct
|
Reopen to discuss the review process in ropensci/software-review#395 |
I tried to reproduce the errors found in the editor's check using my laptop (running Windows) and my server (running ubuntu 16.04) but with no success 😢 IMO, the warning messages are not that important since they are simply caused by different versions of GDAL that do not support WKT (like on my server), while provider's data in but maybe we should also add another note in the package-startup-message. Reprex on the installation: # package is missing
library(osmextract)
#> Error in library(osmextract): there is no package called 'osmextract'
# download pkg from github
temp_zip <- tempfile(fileext = ".zip")
download.file(
url = "https://github.com/ITSLeeds/osmextract/archive/master.zip",
destfile = temp_zip
)
unzip(temp_zip, exdir = tempdir())
# install
devtools::install(paste0(tempdir(), "/osmextract-master"), dependencies = TRUE, build_vignettes = TRUE)
#>
#> checking for file ‘/tmp/RtmpL9uJxp/osmextract-master/DESCRIPTION’ ... ✓ checking for file ‘/tmp/RtmpL9uJxp/osmextract-master/DESCRIPTION’
#> ─ preparing ‘osmextract’:
#> checking DESCRIPTION meta-information ... ✓ checking DESCRIPTION meta-information
#> ─ installing the package to build vignettes
#> creating vignettes ... ✓ creating vignettes (13.7s)
#> ─ checking for LF line-endings in source and make files and shell scripts
#> ─ checking for empty or unneeded directories
#> ─ looking to see if a ‘data/datalist’ file should be added
#> ─ building ‘osmextract_0.1.0.tar.gz’
#>
#> Running /usr/lib/R/bin/R CMD INSTALL /tmp/RtmpL9uJxp/osmextract_0.1.0.tar.gz \
#> --install-tests
#> * installing to library ‘/home/dsuser/R/x86_64-pc-linux-gnu-library/3.6’
#> * installing *source* package ‘osmextract’ ...
#> ** using staged installation
#> ** R
#> ** data
#> *** moving datasets to lazyload DB
#> ** inst
#> ** tests
#> ** byte-compile and prepare package for lazy loading
#> ** help
#> *** installing help indices
#> *** copying figures
#> ** building package indices
#> ** installing vignettes
#> ** testing if installed package can be loaded from temporary location
#> ** testing if installed package can be loaded from final location
#> ** testing if installed package keeps a record of temporary installation path
#> * DONE (osmextract)
# tests
devtools::test(paste0(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
#> ✓ | OK F W S | Context
#> ⠏ | 0 | download⠋ | 0 1 | download⠙ | 1 1 | download⠴ | 5 1 | download✓ | 6 1 | download [0.7 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⠋ | 0 1 | find⠼ | 1 4 | find✓ | 3 4 | find [0.4 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 14 | match⠋ | 12 19 | match⠏ | 24 26 | 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 | read⠹ | 3 | read✓ | 3 1 | read [0.7 s]
#> ────────────────────────────────────────────────────────────────────────────────────────────────────
#> 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.3 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.8 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: 3.4 s
#>
#> OK: 48
#> Failed: 0
#> Warnings: 45
#> Skipped: 1
#>
#> Your tests are tremendous! Created on 2020-09-30 by the reprex package (v0.3.0) @Robinlovelace do you have any suggestion? |
I guess it's a GDAL version issue. In terms of tackling the issue in the package, could we add a version check on package load along the following lines? if(sf::sf_extSoftVersion()["GDAL"] < "3.0.0" ) warning("Not all features are supported, try updating your GDAL version. See https://github.com/r-spatial/sf#installing") It seems to me that on Windows GDAL 3.0.4 is out in the binary install: https://github.com/rwinlib/gdal3 Regarding MacOS, which I guess @annakrystalli (apologies for the tag and thanks ; ) is running, I think updating GDAL as per instructions here have a chance of fixing it: https://github.com/r-spatial/sf#macos. I noticed that some of our functions require recent versions of the 'OSGeo stack', including GEOS and PROJ, so it may also be an issue with some versions of those libraries. Good news: this issue will tend to fade as OSs update (it also affects some versions of Ubuntu I recall from previous CI issues that were fixed by adding the |
Ok, I agree, I will add a note to
Ok thanks, I will try to write an answer after adding that note! |
Hi! I will add here some notes related to the reviews of the package (see ropensci/software-review#395 (comment)). First of all, thank you very much @potterzot for your comments.
In any case, I agree that the warning message could be made clearer, using the term Moreover, I agree that the warning messages could be improved since an important warning could be lost in the "wall of text". I would fix that following the ideas in #133 |
Another comment from me: I think |
Hi @Robinlovelace, can you check the following response? 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 useful functions and arguments. In the following parts, 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] 180538717 Created 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] 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)
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] 33969933 Created on 2021-01-18 by the reprex package (v0.3.0) We added a link between osmextract and Nominatim provider, which implies that 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)
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 |
I suggest adding a link to the issue tracker. |
I suggest a slight reword:
|
Pending those 2 minor suggested changes this is ready to ship 🚢 good work 👍 |
😄 🚀 🎉 |
No description provided.
The text was updated successfully, but these errors were encountered: