Repository navigation
rnaturalearth #22
Description
Activity
@andysouth Thanks for this! Seeking reviewer(s)
I could take a look
Thanks both!
On 26 November 2015 at 23:44, Robin notifications@github.com wrote:
I could take a look
—
Reply to this email directly or view it on GitHub
#22 (comment).Reviewers: @Robinlovelace, @lmullen
Thanks all. I appreciate your efforts.
If you have questions feel free to ask.On 30 November 2015 at 16:17, Scott Chamberlain notifications@github.com
wrote:@Robinlovelace https://github.com/Robinlovelace and @lmullen
https://github.com/lmullen reviewing - thanks both!—
Reply to this email directly or view it on GitHub
#22 (comment).First, let me say that this package is very useful. It will be a very helpful addition to the mapping packages on CRAN and in rOpenSci. I already have plans to use it for my own work and in assignments for students. Well done, @andysouth.
It takes about 2 minutes to install the dev version from GitHub and the installed version is 26 MB. In addition to the fact that CRAN won't accept the package until the data size is smaller, the weight of this package also hinders you as the developer from doing updates as necessary, and it makes it difficult for users to install the package. To solve this it will be necessary to break this package into two or more other packages as detailed below. For the purposes of this review I've focused mainly on package-level issues. Once you've solved the size problem and the package design has stabilized, I'll review the code more granularly.
Structural changes
This package should be at least two packages, and probably three. Here is a suggestion for how you should break this up:
- The main package would be
rnaturalearthand it would be on CRAN. It would be the only package that a user would ever install or load directly. It should contain nothing but the code to download NaturalEarth files, functions to access the data from the other packages, and the vignettes and package level documentation. The absence of data from this package means that you can update it with impunity, since the size of the package will be very small. (I've found this also helps speed up development.) - The second package would be
rnaturalearthDataor the like, and it should also be on CRAN. It would contain nothing but the most essential data: as much as you can fit while keeping the package under the 5MB limit set by CRAN. It will also contain the documentation for the datasets. This package will be imported byrnaturalearth, so users will install it whenever they installrnaturalearth. But since this data will changed infrequently, users will seldom need to update this package. - The third package will be
rnaturalearthHiResor the like. This package would reside solely in the rOpenSci drat repository. It would be a suggested package ofrnaturalearth, using theAdditionalReposfield in the DESCRIPTION. When users call a function that would use this pre-downloaded data, thernaturalearthpackage would check ifrnaturalearthHiResis installed. If it is not installed, then it will ask to install the package from the rOpenSci drat repo. Since this would be a non-CRAN package, you can include as much data as you want. Whether it would ever make sense to break this high resolution package into several different packages would be up to you.
A compromise solution would be to put minimal data into
rnaturalearth, and have all the other data in a single rOpenSci drat repository package. But if you're going to do this, I think you may as well get as well break it up into one further package as suggested above.Now that is a lot of breaking up into multiple packages what will remain, as far as users are concerned, a single user-facing package. But this is the accepted practice for packages which depend on a lot of data. For instance, the
openNLPpackage installs very large packages containing trained models for different langauges from Datacube. I've had to do this ingenderandgenderdata, even though the data there is much smaller than the geographic data you are working with. And I'm in the process of redoingUSAboundariesinto exactly the same package structure that I'm suggesting here. If it is a helpful model, you might take a look at gender and genderdata. Here are the relevant functions.When you do this, you should feel free to give the data packages the CC0 license, but to give the
rnaturalearthpackage which contains only your own work whatever open source license you prefer.@sckott, @karthik, @cboettig: To be able to test these changes effectively, @andysouth will need to use the rOpenSci drat repository. I'd suggest that the package be provisionally accepted and moved into rOpenSci GitHub and drat repositories.
Tooling
- The
README.mdshould be converted toREADME.Rmdviadevtools::use_readme_rmd(). As a bonus, this will let the README include images easily. - The package should use Appveyor's CI for Windows via
devtools::use_appveyor().
DESCRIPTION
- Specify a version number for
rgdal,httr, andknitr.
Tests
All the tests pass. I'll check coverage more carefully once the package structure has stabilized. In the meantime, all of the tests which download files should have
skip_on_cran().Vignette
The vignette should not need download anything from Natural Earth. The code block at the end with two downloads should be set to
eval = FALSE.Code style
There are places where the code style can be improved. See the suggestions in this chapter of Hadley Wickham's R Packages book or the style chapter in his Advanced R book.
- In the README and vignettes, in place of
require(rnaturalearth)uselibrary(rnaturalearth). - Spacing is often inconsistent around
=using in function definitions and calls. There should always be a single space on both sides of=. For example, in the READMEvignette("rnaturalearth", package="rnaturalearth")should bevignette("rnaturalearth", package = "rnaturalearth"). - Comments at the start of the line should be followed by a space. (I prefer to user sentence capitalization; but that's just a preference.) E.g.,
# This is more legible- Most important, all code which has been commented out needs to be deleted.
- The same goes for the data download scripts in
data-raw. At the moment, they have a lot investigating the data. Now that you've settled on what to do, you can retain that information in comments but trim unnecessary code. - Functions which have limited categorical options should specify all the options in the function signature and then use
match.args(). An example of this isget_data()and thetypeparameter. For example, a function that looks like this
myfunc <- function(type = "a") { if(type == "a") { # ... } else if (type =="b") { } else { stop("Error.") } }
Should be changed to this:
myfunc <- function(type = c("a", "b")) { type <- match.arg(type) # Logic checking type here. }
get_data()should be renamed to something less generic, likene_get_data(). Or, can you avoid exporting it?
Human language style
- In the headings of the vignettes, prefer sentence-style capitalization. E.g.
1. Maps in the package. (I'd suggest the same for comments as well, but that's entirely up to you.) - This line in the vignette should be revised, perhaps into a bulleted list, since a sentence can only contain one colon.
contains pre-downloaded vector maps for countries: ne_countries(), states: ne_states() and coastline: ne_coastline()
- The main package would be
Thanks @lmullen for a very helpful review and for giving me clear
guidelines to follow. I knew some of these needed doing, now I know how.
I'll wait for feedback from others and then get stuck in.On 14 December 2015 at 22:31, Lincoln Mullen notifications@github.com
wrote:First, let me say that this package is very useful. It will be a very
helpful addition to the mapping packages on CRAN and in rOpenSci. I already
have plans to use it for my own work and in assignments for students. Well
done, @andysouth https://github.com/AndySouth.It takes about 2 minutes to install the dev version from GitHub and the
installed version is 26 MB. In addition to the fact that CRAN won't accept
the package until the data size is smaller, the weight of this package also
hinders you as the developer from doing updates as necessary, and it makes
it difficult for users to install the package. To solve this it will be
necessary to break this package into two or more other packages as detailed
below. For the purposes of this review I've focused mainly on package-level
issues. Once you've solved the size problem and the package design has
stabilized, I'll review the code more granularly.
Structural changesThis package should be at least two packages, and probably three. Here is
a suggestion for how you should break this up:- The main package would be rnaturalearth and it would be on CRAN. It
would be the only package that a user would ever install or load directly.
It should contain nothing but the code to download NaturalEarth files,
functions to access the data from the other packages, and the vignettes and
package level documentation. The absence of data from this package means
that you can update it with impunity, since the size of the package will be
very small. (I've found this also helps speed up development.) - The second package would be rnaturalearthData or the like, and it
should also be on CRAN. It would contain nothing but the most essential
data: as much as you can fit while keeping the package under the 5MB limit
set by CRAN. It will also contain the documentation for the datasets. This
package will be imported by rnaturalearth, so users will install it
whenever they install rnaturalearth. But since this data will changed
infrequently, users will seldom need to update this package. - The third package will be rnaturalearthHiRes or the like. This
package would reside solely in the rOpenSci drat repository
http://packages.ropensci.org/. It would be a suggested package of
rnaturalearth, using the AdditionalRepos field in the DESCRIPTION.
When users call a function that would use this pre-downloaded data, the
rnaturalearth package would check if rnaturalearthHiRes is installed.
If it is not installed, then it will ask to install the package from the
rOpenSci drat repo. Since this would be a non-CRAN package, you can include
as much data as you want. Whether it would ever make sense to break this
high resolution package into several different packages would be up to you.
A compromise solution would be to put minimal data into rnaturalearth,
and have all the other data in a single rOpenSci drat repository package.
But if you're going to do this, I think you may as well get as well break
it up into one further package as suggested above.Now that is a lot of breaking up into multiple packages what will remain,
as far as users are concerned, a single user-facing package. But this is
the accepted practice for packages which depend on a lot of data. For
instance, the openNLP package installs very large packages containing
trained models for different langauges from Datacube. I've had to do this
in gender and genderdata, even though the data there is much smaller than
the geographic data you are working with. And I'm in the process of redoing
USAboundaries into exactly the same package structure that I'm suggesting
here. If it is a helpful model, you might take a look at gender
https://github.com/ropensci/gender and genderdata
https://github.com/ropensci/genderdata. Here are the relevant functions
https://github.com/ropensci/gender/blob/master/R/install-genderdata-package.R
.When you do this, you should feel free to give the data packages the CC0
license, but to give the rnaturalearth package which contains only your
own work whatever open source license you prefer.@sckott https://github.com/sckott, @karthik https://github.com/karthik,
@cboettig https://github.com/cboettig: To be able to test these changes
effectively, @andysouth https://github.com/AndySouth will need to use
the rOpenSci drat repository. I'd suggest that the package be provisionally
accepted and moved into rOpenSci GitHub and drat repositories.
Tooling- The README.md should be converted to README.Rmd via
devtools::use_readme_rmd(). As a bonus, this will let the README
include images easily. - The package should use Appveyor's CI for Windows via
devtools::use_appveyor().
DESCRIPTION
- Specify a version number for rgdal, httr, and knitr.
Tests
All the tests pass. I'll check coverage more carefully once the package
structure has stabilized. In the meantime, all of the tests which download
files should have skip_on_cran().
VignetteThe vignette should not need download anything from Natural Earth. The
code block at the end with two downloads should be set to eval = FALSE.
Code styleThere are places where the code style can be improved. See the suggestions
in this chapter of Hadley Wickham's R Packages book
http://r-pkgs.had.co.nz/r.html or the style chapter
http://adv-r.had.co.nz/Style.html in his Advanced R book.- In the README and vignettes, in place of require(rnaturalearth) use
library(rnaturalearth). - Spacing is often inconsistent around = using in function definitions
and calls. There should always be a single space on both sides of =.
For example, in the README vignette("rnaturalearth",
package="rnaturalearth") should be vignette("rnaturalearth", package =
"rnaturalearth"). - Comments at the start of the line should be followed by a space. (I
prefer to user sentence capitalization; but that's just a preference.) E.g.,
This is more legible
-
Most important, all code which has been commented out needs to be
deleted.
The same goes for the data download scripts in data-raw. At the
moment, they have a lot investigating the data. Now that you've settled on
what to do, you can retain that information in comments but trimunnecessary code.
Functions which have limited categorical options should specify all
the options in the function signature and then use match.args(). An
example of this is get_data() and the type parameter. For example, a
function that looks like this
myfunc <- function(type = "a") {
if(type == "a") {
# ...
} else if (type =="b") {} else {
stop("Error.")
}
}Should be changed to this:
myfunc <- function(type = c("a", "b")) {
type <- match.arg(type)Logic checking type here.
}
- get_data() should be renamed to something less generic, like
ne_get_data(). Or, can you avoid exporting it?
Human language style
- In the headings of the vignettes, prefer sentence-style
capitalization. E.g. 1. Maps in the package. (I'd suggest the same for
comments as well, but that's entirely up to you.) - This line in the vignette should be revised, perhaps into a bulleted
list, since a sentence can only contain one colon. contains
pre-downloaded vector maps for countries: ne_countries(), states:
ne_states() and coastline: ne_coastline()
—
Reply to this email directly or view it on GitHub
#22 (comment).- The main package would be rnaturalearth and it would be on CRAN. It
Review
A number of packages provide geographic data, but none has yet made the high quality datasets within the naturalearth project easily available to R users. rnaturalearth does this with a user-friendly command-line interface, which encourages exploration of what the collection has to offer, in addition to simply providing country and 'state' outlines at various level of geographical resolution. The package will very useful for anyone wanting to make simple yet accurate maps quickly.
ROpenSci, with its emphasis on data access, is an ideal community space to host this package.
Useability of the functions
Overall I found the package intuitive and easy to use. The preface of
ne_is useful for quickly accessing the package's functions (in a similar way that stringr functions begin withst_) and I think the order and description of the function arguments makes sense, given the structure of the data on the naturalearth data repository.The main vignette succintly describes the core purpose and functions of the package, although I think more on what extra can be downloaded via
ne_download()would be interesting here. Thewhat-is-a-countryvignette provides a useful discussion of the subtleties surrounding up how to divide up geographical space along national, state and regional borders.Minor comments/suggestions:
- I think the description of the
@paramtypeinne_downloadcould be better at conveying to the user what datasets are available. It is not clear from the function documentation, for example, that lakes can be downloaded withne_download(type = 'lakes', category = 'physical'). To resolve this issue I suggest that a link to the dataset types is make more explicit (e.g. "see naturalearthdata.com/downloads/110m-cultural-vectors/ for the types of data that can be downloaded for cultural 110 m resolution data.") - To assist with the 'making it clear what data is available' issue, I think that
ne_downloadcould usefully be split intone_download10,ne_download50andne_download110, while keeping the genericne_downloadfunction intact. The data available varies depending on scale, sone_download10, for example, could provide descriptions of the very useful roads and railways datasets. It was not immediately obvious that these very interesting datasets were made available with - this could usefully be better documented.
roads <- ne_download(scale = 10, type = "roads", category = "cultural") railroads <- ne_download(scale = 10, type = "railroads", category = "cultural")
spis an import but I think the package would benefit from making it a dependency (as it is with stplanr after some debate): all the data loaded by the package that I've seen is in an sp S4 object. Yet these will not work (e.g. plot) properly unless sp is loaded, as illustrated in the vignette. This would also make the plot commands in the vignette simpler.scale = 10isn't listed as an argument ofne_countries("scale of map to return, one of 110, 50, 'small', 'medium'") but it works so should be added.- It is not clear that
categoryis a needed argument.airports <- ne_download(scale = 10, type = "airports"), for example, works the same asairports <- ne_download(scale = 10, type = "airports")- there are no types I can see that duplicate names between categories. - I couldn't see how to download ice shelf data succesfully. I was expecting
ne_download(scale = 10, type = "antarctic_ice_shelves", category = "physical")orne_download(scale = 10, type = "antarctic ice shelves", category = "physical")to work but neither seemed to... - Links to other data sources such as the SRTM (some of which can be accessed through the raster package) would be useful, perhaps in the vignette.
Thoughts on reducing the package size
In-line with the previous reviewer I think the size of the package could be problematic. I think the suggestion of splitting it into other packages is a good one. An alternative would be to provide a default destination within the package for data download, not host any data on the package itself directly, and simply use the package to access the data hosted on rnaturalearth. Other than package install requirements, the clear advantage of that is that the data won't have to be updated. The disadvantage is that the user would need internet access the first time they accessed many datasets and possible complexities surrounding fresh installs wiping previously downloaded data.
if(file.exists(...))could be used to decide whether the data should be downloaded or not. Below is an illustrative example for what the call could look like something like this, forne_countries():ne_countries_10 <- function(){ file_path <- file.path(system.file("data", package = "rnaturalearth"), "countries10.rda") if(file.exists(file_path)){ x <- readRDS(file_path) }else{ x <- ne_download() saveRDS(x, file_path) } x }
Note that
datamay need to change toexdatain the above code and that this is just a preliminary idea - would be interested to hear thoughts from others. In any case I can see a disadvantage of hosting raw data on CRAN or ROpenSci in terms of the DRY principle.I'm not sure about specifying providing the same data with
data(countries110)is necessary given tcountries10 <- ne_countries(110)does the same job.I'd like to point out that I'm not an expert in this area, think the previous reviewers' suggestions in relation to package size is also another viable option and would be interested in hearing views from others, especially in relation to the DRY principle as it applies to data.
- I think the description of the
Thanks @Robinlovelace https://github.com/Robinlovelace for a considered
and useful review.On 17 December 2015 at 00:15, Robin notifications@github.com wrote:
Review
A number of packages provide geographic data, but none has yet made the
high quality datasets within the naturalearth project easily available to R
users. rnaturalearth does this with a user-friendly command-line interface,
which encourages exploration of what the collection has to offer, in
addition to 'simply' providing country and 'state' outlines at various
level of geographical resolution. The package will very useful for anyone
wanting to make simple yet accurate maps quickly.ROpenSci, with its emphasis on data access, is an ideal community space to
host this package.
Useability of the functionsOverall I found the package intuitive and easy to use. The preface of ne_
is useful for quickly accessing the package's functions (in a similar way
that stringr functions begin with st_) and I think the order and
description of the function arguments makes sense, given the structure of
the data on the naturalearth data repository.The main vignette
https://github.com/AndySouth/rnaturalearth/blob/master/vignettes/rnaturalearth.Rmd
succintly describes the core purpose and functions of the package, although
I think more on what extra can be downloaded via ne_download() would be
interesting here. The what-is-a-country vignette provides a useful
discussion of the subtleties surrounding up how to divide up geographical
space along national, state and regional borders.Minor comments/suggestions:
I think the description of the @PARAM type in ne_download could be
better at conveying to the user what datasets are available. It is not
clear from the function documentation, for example, that lakes can be
downloaded with ne_download(type = 'lakes', category = 'physical'). To
resolve this issue I suggest that a link to the dataset types is make more
explicit (e.g. "see
naturalearthdata.com/downloads/110m-cultural-vectors/
http://www.naturalearthdata.com/downloads/110m-cultural-vectors/ for
the types of data that can be downloaded for cultural 110 m resolution
data.")To assist with the 'making it clear what data is available' issue, I
think that ne_download could usefully be split into ne_download10,
ne_download50 and ne_download110, while keeping the generic ne_download
function intact. The data available varies depending on scale, so
ne_download10, for example, could provide descriptions of the very
useful roads and railways datasets. It was not immediately obvious that
these very interesting datasets were made available with - this could
usefully be better documented.roads <- ne_download(scale = 10, type = "railroads", category = "cultural")railroads <- ne_download(scale = 10, type = "railroads", category = "cultural")
sp is a dependency but I think the package would benefit from making
it a dependency: all the data loaded by the package that I've seen is in an
sp S4 object. Yet these will not work (e.g. plot) properly unless sp is
loaded, as illustrated in the vignette. This would also make the plot
commands in the vignette simpler.scale = 10 isn't listed as an argument of ne_countries("scale of map
to return, one of 110, 50, 'small', 'medium'") but it works so should be
added.It is not clear that category is a needed argument. airports <-
ne_download(scale = 10, type = "airports"), for example, works the
same as airports <- ne_download(scale = 10, type = "airports") - there
are no types I can see that duplicate names between categories.I couldn't see how to download ice shelf data succesfully. I was
expecting ne_download(scale = 10, type = "antarctic_ice_shelves",
category = "physical") or ne_download(scale = 10, type = "antarctic
ice shelves", category = "physical") to work but neither seemed to...Links to other data sources such as the SRTM (some of which can be
accessed through the raster package) would be useful, perhaps in the
vignette.Thoughts on reducing the package size
In-line with the previous reviewer I think the size of the package could
be problematic. I think the suggestion of splitting it into other packages
is a good one. An alternative would be to provide a default destination
within the package for data download, not host any data on the package
itself directly, and simply use the package to access the data hosted on
rnaturalearth. Other than package install requirements, the clear advantage
of that is that the data won't have to be updated. The disadvantage is that
the user would need internet access the first time they accessed many
datasets and possible complexities surrounding fresh installs wiping
previously downloaded data.if(file.exists(...)) could be used to decide whether the data should be
downloaded or not. Below is an illustrative example for what the call could
look like something like this, for ne_countries():ne_countries_10 <- function(){
file_path <- file.path(system.file("data", package = "rnaturalearth"), "countries10.rda")
if(file.exists(file_path)){
x <- readRDS(file_path)
}else{
x <- ne_download()
saveRDS(x, file_path)
}
x
}Note that data may need to change to exdata in the above code and that
this is just a preliminary idea - would be interested to hear thoughts from
others. In any case I can see a disadvantage of hosting raw data on CRAN or
ROpenSci in terms of the DRY principle.I'm not sure about specifying a function to generate data ne_countries()
while
providing the same data with data(countries110), and the type argument is
necessary, especially given the suggestion above of a way to reduce the
package's size whilst maintaining the downloaded data in a default and
accessible location.I'd like to point out that I'm not an expert in this area, think the
previous reviewers' suggestions in relation to package size is also another
viable option and would be interested in hearing views from others,
especially in relation to the DRY principle as it applies to data.—
Reply to this email directly or view it on GitHub
#22 (comment).42 remaining items
I've forked to my account to see if I can sort it out, will report back if successful
Reacted by Eduard SzöcsHi @sckott does it seem that Edzers solution didn't work either ?
https://github.com/sckott/rnaturalearth/commit/68f30e781d42ecd5c312d1caf2483e35c7dd74c3Because it succeeds on appveyor does this suggest it's a specific Travis problem ? Should this stop proceeding to rOpenSci & CRAN ? I'm keen to proceed if possible.
@andysouth Yeah, let's move on, couldn't sort it out yet, but not crucial as it seems to pass elsewhere just not on travis.
Approved (see new label on issue)
- Add this footer to your README:
[](http://ropensci.org)- Update installation of dev versions to
ropenscilabs/rnaturalearthand any urls for the github repo toropenscilabsinstead ofAndySouth - Go to the Repo Settings --> Transfer Ownership and transfer to
ropenscilabs- Note that all our newer pkgs go toropenscilabsfirst, then when more mature we'll move toropensci
you should get an invitation to a team within ropenscilabs
rnaturalearth provides :
https://github.com/AndySouth/rnaturalearth
www.naturalearthdata.com
Anyone who wants to use Natural Earth vector data to make a map or perform other geographical analyses.
More limited subsets of Natural Earth data are available in at least the following CRAN packages rworldmap, rworldxtra, choroplethr, maps, oce, tmap. rnaturalearth is different in that it provides a comprehensive reproducible solution for access to Natural Earth vector map data either pre-downloaded or by facilitating download. By separating data access from visualisation this package provides a resource that can be used by other visualisation packages.
devtools::check()produce any errors or warnings? If so paste them below.One NOTE
installed size is 26.0Mb
sub-directories of 1Mb or more:
data 25.8Mb
The package is currently too big because of the fine scale (10m) data. Hadley suggested :
"I think you need to keep the data under five meg. I'd suggest that you put the fine level data a separate package - you can then make that available via a drat repo (perhaps the ROpenSci one?), or just via github."
I don't know anything about drat repos. I'm happy to take advice and modify the package(s) as recommended.