Skip to content
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

mregions: Marine Regions Data from Marineregions.org #53

Closed
10 tasks done
sckott opened this issue Jun 16, 2016 · 18 comments
Closed
10 tasks done

mregions: Marine Regions Data from Marineregions.org #53

sckott opened this issue Jun 16, 2016 · 18 comments

Comments

@sckott
Copy link
Contributor

sckott commented Jun 16, 2016

    1. What does this package do? (explain in 50 words or less)
      mregions is a wrapper to REST API methods from http://www.marineregions.org/ - with purpose of fetching marine regions in geojson/shp/wkt format for use downstream.
    1. Paste the full DESCRIPTION file inside a code block below.
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
    1. URL for the package (the development repository, not a stylized html page)
      https://github.com/ropenscilabs/mregions
    1. What data source(s) does it work with (if applicable)?
      REST API from http://www.marineregions.org/ and a geoserver API from a related service http://geo.vliz.be/geoserver/ows
    1. Who is the target audience?
      Biologists/oceanographers/ecologists and anyone else wanting to overlay data, or clip data to, various defined marine regions
    1. Are there other R packages that accomplish the same thing? If so, what is different about yours?
      Not that I know of
    1. Check the box next to each policy below, confirming that you agree. These are mandatory.
    • This package does not violate the Terms of Service of any service it interacts with.
    • The repository has continuous integration with Travis CI and/or another service
    • The package contains a vignette
    • The package contains a reasonably complete README with instructions for installing the development version.
    • The package contains unit tests
    • The package only exports functions to the NAMESPACE that are intended for end users
    1. Do you agree to follow the rOpenSci packaging guidelines? These aren't mandatory, but we strongly suggest you follow them. If you disagree with anything, please explain.
    • Are there any package dependencies not on CRAN? No
    • Do you intend for this package to go on CRAN? Yes
    • Does the package have a CRAN accepted license? Yes
    • Did R CMD check (or devtools::check()) produce any errors or warnings? If so paste them below.
    1. Please add explanations below for any exceptions to the above:
    1. If this is a resubmission following rejection, please explain the change in circumstances.
@sckott sckott added the package label Jun 16, 2016
@karthik
Copy link
Member

karthik commented Jun 16, 2016

Happy to review

@karthik
Copy link
Member

karthik commented Jun 16, 2016

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

@noamross
Copy link
Contributor

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

Seeking reviewers...

@noamross
Copy link
Contributor

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

Still seeking second reviewer

@mdsumner
Copy link

mdsumner commented Jun 24, 2016

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.

@noamross
Copy link
Contributor

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

@sckott
Copy link
Contributor Author

sckott commented Jun 24, 2016

thanks a lot @mdsumner ! will make changes very soon

@noamross noamross self-assigned this Jul 11, 2016
@sckott
Copy link
Contributor Author

sckott commented Jul 13, 2016

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

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

true, i'll do that (ropensci-archive/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-archive/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-archive/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-archive/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-archive/mregions#20)

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

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

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

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

@sckott
Copy link
Contributor Author

sckott commented Jul 26, 2016

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

Anything else @mdsumner @noamross ?

@sckott
Copy link
Contributor Author

sckott commented Jul 27, 2016

anything else I should do @mdsumner ?

@noamross
Copy link
Contributor

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.

@sckott
Copy link
Contributor Author

sckott commented Jul 28, 2016

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-archive/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-archive/mregions#29

@sckott
Copy link
Contributor Author

sckott commented Aug 1, 2016

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
Copy link

mdsumner commented Aug 2, 2016

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

@mdsumner
Copy link

mdsumner commented Aug 2, 2016

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

Cheers, Mike

@noamross
Copy link
Contributor

noamross commented Aug 2, 2016

Thank you @mdsumner and @sckott. Approved!

@sckott
Copy link
Contributor Author

sckott commented Aug 2, 2016

cool, now to go to CRAN

@sckott
Copy link
Contributor Author

sckott commented Aug 2, 2016

thanks for your review @mdsumner !

@noamross noamross closed this as completed Aug 2, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants