search

ropensci / software-review

rOpenSci Software Peer Review.
292 stars 104 forks source link

mregions: Marine Regions Data from Marineregions.org #53

Closed sckott closed 8 years ago

sckott commented 8 years ago 
Package: mregions
Title: Marine Regions Data from Marineregions.org
Description: Tools to get marine regions data from Marineregions.org.
    Includes tools to get region metadata, as well as data in GeoJSON
    format, as well as Shape files.
Version: 0.0.9.9400
License: MIT + file LICENSE
Authors@R: c(
    person("Scott", "Chamberlain", role = c("aut", "cre"), email = "myrmecocystus@gmail.com"),
    person("Pieter", "Provoost", role = "aut")
    )
URL: https://github.com/ropenscilabs/mregions
BugReports: https://github.com/ropenscilabs/mregions/issues
Imports:
    httr (>= 1.1.0),
    jsonlite (>= 0.9.20),
    xml2,
    wellknown,
    rappdirs
Suggests:
    testthat,
    geojsonio,
    rgdal,
    rgeos,
    knitr
Enhances: leaflet
VignetteBuilder: knitr
RoxygenNote: 5.0.1
karthik commented 8 years ago

Happy to review

karthik commented 8 years ago

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

noamross commented 8 years ago

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

Seeking reviewers...

noamross commented 8 years ago

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

Still seeking second reviewer

mdsumner commented 8 years ago

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

library(mregions)
rn <- region_names()
library(dplyr)
bind_rows(region_names_search("EEZ")) %>% select(title)
do.call(rbind, lapply(region_names(), as.data.frame, stringsAsFactors = FALSE))

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

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

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

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.

noamross commented 8 years ago

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

sckott commented 8 years ago

thanks a lot @mdsumner ! will make changes very soon

sckott commented 8 years ago

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 (ropenscilabs/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 (ropenscilabs/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() (ropenscilabs/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 (ropenscilabs/mregions#16)

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

true, i'll do that (ropenscilabs/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

(ropenscilabs/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 (ropenscilabs/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 (ropenscilabs/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 (ropenscilabs/mregions#20)

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

Will do (ropenscilabs/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 (ropenscilabs/mregions#22)

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

good idea (ropenscilabs/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 (ropenscilabs/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 (ropenscilabs/mregions#25)

sckott commented 8 years ago

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

Anything else @mdsumner @noamross ?

sckott commented 8 years ago

anything else I should do @mdsumner ?

noamross commented 8 years ago

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

sckott commented 8 years ago

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! https://github.com/ropenscilabs/mregions/issues/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 ?:

https://github.com/ropenscilabs/mregions/issues/29

sckott commented 8 years ago

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

mdsumner commented 8 years ago

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 notifications@github.com 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 https://github.com/ropensci/onboarding/issues/53#issuecomment-236732599, 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

mdsumner commented 8 years ago

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

Cheers, Mike

noamross commented 8 years ago

Thank you @mdsumner and @sckott. Approved!

sckott commented 8 years ago

cool, now to go to CRAN

sckott commented 8 years ago

thanks for your review @mdsumner !