Comments (18)
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
- 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-runsregion_names
, which is a bit wasteful, it could accept already-obtained output of region_names as an optional inputagrep
is used inregion_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 thanmregions
, 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 themregions_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 ofregion_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 inregion_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
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()
inrev_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 forsapply()
as output is discarded. Switch tolapply()
? - 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
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.
Related Issues (20)
- gigs: Assess Fetal, Newborn, and Child Growth with International Standards HOT 8
- dendroNetwork HOT 50
- eDNAjoint: an R package for interpreting paired environmental DNA and traditional surveys HOT 16
- Capybara HOT 9
- Presubmission inquiry: `chopin` HOT 2
- Presubmission enquiry for cocoon: Extract, Format, and Print Statistical Output HOT 3
- QuadratiK: A Collection of Methods Using Kernel-Based Quadratic Distances for Statistical Inference and Clustering HOT 52
- osmapiR: an implementation of OpenStreetMap API v0.6 for R HOT 89
- ## Reply to package review by @njtierney HOT 3
- cancerprof: API Client for extracting data from State Cancer Profiles HOT 3
- rsi: Efficiently Retrieve and Process Satellite Imagery HOT 44
- cancerprof: API Client for extracting data from State Cancer Profiles HOT 41
- `chopin`: Computation for Climate and Health research On Parallelized INfrastructure HOT 54
- rpigpior HOT 13
- phangorn HOT 2
- eDNAjoint: R package for interpreting paired environmental DNA and traditional surveys HOT 44
- Presubmission Inquiry of the `cryptoQuotes`-package HOT 6
- Capybara HOT 22
- QuickJSR - Portable, lightweight JavaScript engine for R
- feowR: Download shapefiles of Freshwater Ecoregions of the World (FEOW) HOT 12
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from software-review.