Oops that's mine, I'll have a look.

ast 13 lines of output:
3: .ensureIsLonlat(x)
4: sp::spTransform(x, "+init=EPSG:4326")
5: sp::spTransform(x, "+init=EPSG:4326")
6: spTransform(x, CRS(CRSobj), ...)
7: CRS(CRSobj)
8: stop(res[[2]])

testthat results

OK: 0 SKIPPED: 11 FAILED: 1

1. Error: conversion to wkt results in longlat data
   (@test-as_wkt_projection.R#7)

On Tue, 2 Aug 2016 at 08:55 Scott Chamberlain [email protected]
wrote:

pretty much ready to go except there's a weird problem in the test suite
only on CI seemingly related tosp, but not quite sure

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#53 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AD6tbxFHH_T-Q34nH3gqfT_JOtluAmXvks5qbnlsgaJpZM4I3mh6
.

Dr. Michael Sumner
Software and Database Engineer
Australian Antarctic Division
203 Channel Highway
Kingston Tasmania 7050 Australia

from software-review.

Hi, I'm satisfied that the issues from my review have been addressed. Good job!

Cheers, Mike

from software-review.

Happy to review

from software-review.

Actually I should withdraw since this is COI for me especially in the context of JOSS. Thanks for the ping @noamross

from software-review.

Passes all editors' checks, seems like a good fit 😉

Seeking reviewers...

from software-review.

Reviewers: @mdsumner
Due date: 2016-07-08

Still seeking second reviewer

from software-review.

Comments

The mregions package provides facilities to easily obtain data from http://www.marineregions.org/ with helpers for working with the data, and including specialist tools for the geo-spatial forms of data (GeoJSON and shapefiles).

The package is structured well using the modern approach with devtools and roxygen2, and using all the ROpenSci recommended development tools, including version control, automated unit-testing, continuous integration, README and vignettes.

The package is easy to use and seems to do all the things that it's designed to do. I found it hard to see what the package is for overall, since the readme, vignette and documentation are all quite sparse and there's no overall "why this?" message. That may not matter if this is specifically for users of Marine Regions, but it seems there are a lot more potential users.

I am very interested personally in how this fits in with the broader geo-spatial ecosystem in R, and will be happy to contribute in an ongoing way - unfortunately this is all I can give before the review deadline. (I had planned on pull requests for some suggestions, but travel plans mean I just can't get to those in time.)

Thanks very much!

Installation

The package can be checked out and built easily in RStudio with devtools. It passes check on Windows and Debian, also passes check on devel on Windows.

Specific comments on code

• I was a little unclear on why we want to convert to WKT, but I see that it's for robis::occurrence. The reasoning for the conversion could be explained.
• region_names_search re-runs region_names, which is a bit wasteful, it could accept already-obtained output of region_names as an optional input
• agrep is used in region_names_search for fuzzy matching, but no control is given to the user over how fuzzy. Is it too fuzzy to get "EEA" from "EEZ"?

library(mregions)
rn <- region_names()
library(dplyr)
bind_rows(region_names_search("EEZ")) %>% select(title)

• get_region_names returns a list, but this might as well be a dataframe (?)

do.call(rbind, lapply(region_names(), as.data.frame, stringsAsFactors = FALSE))

• This item is about robis rather than mregions, but it's important enough to the example used that I think it needs attention. robis::occurrence expects geometry using only longitude/latitude, but as_wkt does not do anything different with a projected Spatial object. It should probably error or auto-transform, rgdal is in use so that's not difficult, (raster has good heuristics for seeing if the assumption of longlat is ok, in the case that an unprojected Spatial object is given without an explicit proj4string). I'm happy to contribute if needed.

This example results in all(?) possible records being queried.

#' devtools::install_github("iobis/robis")
library('robis')
shp <- region_shp(name = "Belgian Exclusive Economic Zone")
## a rogue user decides to load a projected map somehow
shp <- sp::spTransform(shp, "+proj=laea +ellps=WGS84 +lon_0=2.5 +lat_0=51.6")
wkt <- as_wkt(shp)
## now we are in trouble...
xx <- occurrence("Abra alba", geometry = wkt)
## Retrieved 2000 records of 33188 (6%)
## ...

Questions

• the makefile install for the vignette, is that because you cannot automatically build the figure snapshots from source code, or because robis is not on CRAN? Should mregions list robis as a Suggests dependency and be listed in Remotes? (I may be unclear on the scope of review here, and what's appropriate for ROpenSci versus CRAN).

ROpensci criteria

Package name, Function and variable naming, Coding style, Code of Conduct, Testing, Package scaffolding, all good.

Readme

The readme does not describe what the package is for, beyond "get data from ..."

Documentation

The documentation is very terse, functions do not have Details or Value slots, a few explicit sentences in each would help a lot

• as_wkt documentation says it accepts output of "region_geojson" but it also accepts a shapefile name, or a Spatial object
• I suggest to change address of Marine Regions site to "http://www.marineregions.org/"

There is a vignette, but it's a little terse and some of the pre-amble is about installing and is not relevant afaics (move that to readme?).

• vignette("mregions_vignette") is hard to remember, perhaps just call the vignette "mregions"?
• res is used as the example object name for a few different kinds of objects in the mregions_vignette, so there is a lack of clarity on what the workflow is in the sequence of examples.
• The mregions_vignette lists iobis/robis but not how to install it, and the vignette uses it but only in "comment" mode. It's not absolutely obvious that robis is a dependency for the vignette material

Examples

These are good if a little terse in places.

Console messages

There aren't many of these, but I have not had time to explore in much detail.

from software-review.

Thank your for the review @mdsumner! @sckott will make updates and post a response.

from software-review.

thanks a lot @mdsumner ! will make changes very soon

from software-review.

responses to @mdsumner ---> links to issues opened in ropenscilabs/mregions

I found it hard to see what the package is for overall, since the readme, vignette and documentation are all quite sparse and there's no overall "why this?" message. That may not matter if this is specifically for users of Marine Regions, but it seems there are a lot more potential users.

Good point, i'll add some more why you should care bits to various places in the pkg docs (ropensci/mregions#13)

I am very interested personally in how this fits in with the broader geo-spatial ecosystem in R, and will be happy to contribute in an ongoing way - unfortunately this is all I can give before the review deadline.

Looking forward to any contributions!

I was a little unclear on why we want to convert to WKT, but I see that it's for robis::occurrence. The reasoning for the conversion could be explained.

Right, and WKT is a common geospatial data format, often used to store spatial data in databases, and a number of species occurrence databases expect WKT, including OBIS and the biggest one http://www.gbif.org/ - I'll include more reasoning for this (ropensci/mregions#14)

region_names_search re-runs region_names, which is a bit wasteful, it could accept already-obtained output of region_names as an optional input

Good idea, can have it accept either a string or the output of region_names() (ropensci/mregions#15)

agrep is used in region_names_search for fuzzy matching, but no control is given to the user over how fuzzy. Is it too fuzzy to get "EEA" from "EEZ"?

I can add ... to the fxn (ropensci/mregions#16)

region_names returns a list, but this might as well be a dataframe (?)

true, i'll do that (ropensci/mregions#17)

robis::occurrence expects geometry using only longitude/latitude, but as_wkt does not do anything different with a projected Spatial object. ...

Please do contribute if you have time, if not, let me know and I'll try

the makefile install for the vignette, is that because you cannot automatically build the figure snapshots from source code, or because robis is not on CRAN? Should mregions list robis as a Suggests dependency and be listed in Remotes? (I may be unclear on the scope of review here, and what's appropriate for ROpenSci versus CRAN).

it's probably controversial but I don't like vignettes building on package installation, no other languages do this, so i pre-render the vignette to markdown

I just rembered that robis is not on CRAN, and it's out of my control, so not sure when it will be there. I'll use a different package that is on CRAN for examples/vignette

(ropensci/mregions#18)

The readme does not describe what the package is for, beyond "get data from ..."

Good point, will add more info to readme about what it's for (ropensci/mregions#13)

The documentation is very terse, functions do not have Details or Value slots, a few explicit sentences in each would help a lot

Thanks, will make improvements (ropensci/mregions#19)

as_wkt documentation says it accepts output of "region_geojson" but it also accepts a shapefile name, or a Spatial object

Good catch, i'll fix that (ropensci/mregions#20)

I suggest to change address of Marine Regions site to "http://www.marineregions.org/"

Will do (ropensci/mregions#21)

There is a vignette, but it's a little terse and some of the pre-amble is about installing and is not relevant afaics (move that to readme?).

I'll improve the vignette (ropensci/mregions#22)

vignette("mregions_vignette") is hard to remember, perhaps just call the vignette "mregions"?

good idea (ropensci/mregions#23)

res is used as the example object name for a few different kinds of objects in the mregions_vignette, so there is a lack of clarity on what the workflow is in the sequence of examples.

not sure what you mean, but I'll use different object names to avoid any confusion (ropensci/mregions#24)

The mregions_vignette lists iobis/robis but not how to install it, and the vignette uses it but only in "comment" mode. It's not absolutely obvious that robis is a dependency for the vignette material

I just rembered that robis is not on CRAN, and it's out of my control, so not sure when it will be there. I'll use a different package that is on CRAN for examples/vignette

Examples - These are good if a little terse in places

Good point, I'll add more examples (ropensci/mregions#25)

from software-review.

All issues mentioned above in ropenscilabs/mregions have been addressed and closed

Anything else @mdsumner @noamross ?

from software-review.

anything else I should do @mdsumner ?

from software-review.

Just a few comments in addition to @mdsumner's nice review. @mdsumner, let us know if @sckott has addressed your concerns to satisfaction.

• You're probably aware your tests are currently failing on Travis, with

 Error: conversion to wkt results in longlat data (@test-as_wkt_projection.R#8) 

I don't get this locally

• There's also a WARNING that I assume is due to the vignette not being prepared for CRAN yet.
• Is there a reason we don't have test coverage in CI?
• Super nitpicky, mostly to try something out: I ran Gabor's new goodpractice package on this, and in addition to some complaints about test coverage and long lines, it found a usage of sapply() in rev_geo_code(). It's a bit of an odd call, as the output is discarded because this is actually applying a type-checking assertion. I guess there's no reason for sapply() as output is discarded. Switch to lapply()?
• This is just a suggestion, but as there are a variety of geocoding and geoprocessing packages, and their tasks are similar, might it be better to have a function naming scheme that starts with mr_? This way its clear in one's code that one is querying for marine information.

from software-review.

nice, thanks @noamross

You're probably aware your tests are currently failing on Travis, with

I'll look into that, thanks

There's also a WARNING that I assume is due to the vignette not being prepared for CRAN yet.

right, a leaflet warning, i'll install on travis

Is there a reason we don't have test coverage in CI?

don't know why either, can add that.

sapply() in rev_geo_code() ... Switch to lapply()?

yeah, good thinking! ropensci/mregions#28

might it be better to have a function naming scheme that starts with mr_? This way its clear in one's code that one is querying for marine information.

i think so, so ?:

• as_wkt -> mr_as_wkt
• geo_code -> mr_geo_code
• obis_eez_id -> mr_obis_eez_id
• place_relations -> mr_place_relations
• place_types -> mr_place_types
• records_by_type -> mr_records_by_type
• region_geojson -> mr_region_geojson
• region_names -> mr_region_names
• region_names_search -> mr_region_names_search
• region_shp -> mr_region_shp
• rev_geo_code -> mr_rev_geo_code

ropensci/mregions#29

from software-review.

pretty much ready to go except there's a weird problem in the test suite only on CI seemingly related tosp, but not quite sure

from software-review.

Thank you @mdsumner and @sckott. Approved!

from software-review.

cool, now to go to CRAN

from software-review.

thanks for your review @mdsumner !

from software-review.