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

stplanr #10

Closed
6 of 8 tasks
Robinlovelace opened this issue Apr 8, 2015 · 55 comments
Closed
6 of 8 tasks

stplanr #10

Robinlovelace opened this issue Apr 8, 2015 · 55 comments

Comments

@Robinlovelace
Copy link
Member

Robinlovelace commented Apr 8, 2015

    1. What does this package do? (explain in 50 words or less)
      This package brings together a number of tools that enable transport modelling and planning and R, with an emphasis on analyses needed for sustainability (e.g. planning new bicycle paths).
    1. Paste the full DESCRIPTION file inside a code block (bounded by ``` on either end).
Package: stplanr
Type: Package
Title: Sustainable Transport Planning with R
Version: 0.0.1.1
Date: 2015-10-18
Authors@R: c(
    person("Robin", "Lovelace", email = "rob00x@gmail.com", role = c("aut", "cre")),
    person("Richard", "Ellison", role = c("aut"), comment = "Author of various functions"),
    person("Barry", "Rowlingson", role = c("aut"), comment = "Author of overline"),
    person("Nick", "Bearman", role = c("aut"), comment = "Co-author of gclip")
    )
Description: Functionality and data access tools for transport planning, including origin-destination analysis, route allocation and modelling travel patterns.
License: MIT + file LICENSE
BugReports: https://github.com/robinlovelace/stplanr/issues
LazyData: yes
Depends:
    sp, R (>= 3.0)
Imports:
    jsonlite,
    maptools,
    raster,
    rgdal,
    rgeos,
    dplyr,
    RgoogleMaps,
    openxlsx,
    leaflet,
    httr
Suggests:
    testthat,
    knitr,
    tmap
VignetteBuilder: knitr
URL: https://github.com/Robinlovelace/stplanr
    1. URL for the package (the development repository, not a stylized html page)
      https://github.com/Robinlovelace/stplanr
    1. What data source(s) does it work with (if applicable)?
      Example data on route allocation and travel flows (included - more planned), geographical boundaries, transport networks, example travel survey data (to be added subsequently)
    1. Who is the target audience?
      Transport researchers and modellers from academic, public and private sectors. University students studying transport geography, transport modelling and related disciplines.
    1. Check the box next to each policy below, confirming that you agree. These are mandatory.
  • The repository has continuous integration with Travis and/or another service
  • The package contains a vignette
  • The package contains a reasonably complete readme with devtools install instructions
  • The package contains unit tests
    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 packages that your package depends on that are not on CRAN?
  • Do you intend for this package to go on CRAN?
  • Does the package have a CRAN accepted license?
  • Did devtools::check() produce any errors or warnings? If so paste them below.
    1. Please add explanations below for any exceptions to the above:
      This is work in progress and I am learning how to build R packages as I progress. These issues will be fixed, probably during a course on R package development I will attend.
    1. If this is a resubmission following rejection, please explain the change in cirucmstances.
@sckott
Copy link
Contributor

sckott commented Apr 13, 2015

hi @Robinlovelace - Thanks for your submission.

Did you mean to check off some of those things? E.g., you checked that you have continuous integration, but you don't, and that you have tests, but at least we don't see any.

Can you correct the information above?

@Robinlovelace
Copy link
Member Author

Robinlovelace commented Apr 13, 2015

@sckott I need to learn more about 'continuous integration' and tests. I'm attending a 3 day course on R package development in May. Proposal: I close this issue for now and re-open once I know more about those things and have had more feedback about the package in general. And I'll reduce the number of check() warnings! Sound good? Any advice appreciated. The tool is already being used for real-world applications and I'm sure it will be useful, once it's a little more polished.

@sckott
Copy link
Contributor

sckott commented Apr 13, 2015

@Robinlovelace Up to you if you want to close the issue and open a new one later.

Where are users expected to get data for use in functions from your package? If data is publicly available (e.g., via web APIs, or even just ftp), we can help build in some functions to provide data access.

I'm attending a 3 day course on R package development in May.

cool, sounds good!

The R CMD CHECK warnings aren't a huge deal - we can help with those. Bigger issues are:

  • fit for ropensci - if there is data access functionality in your package (discussed above), it would be a better fit for us
  • vignettes/help for users: this is pretty crucial - we want it to be easy for users to figure out how to use a package
  • tests - tests are pretty crucial to make sure any functionality of the package doesn't break when something is changed

@karthik
Copy link
Member

karthik commented Apr 13, 2015

@Robinlovelace Just wanted to mentioned that we can also help with some of these issues. If you tag one of us in your repo, we can help with CI, testing and other CHECK issues. And as we go along we can add this to our knowledgebase (on the wiki for this repo) so that others in the future can benefit (and also streamline the process).

@sckott sckott added the holding label Apr 13, 2015
@sckott
Copy link
Contributor

sckott commented Apr 13, 2015

@Robinlovelace let's leave this issue open for now. I added the holding tag to indicate that we're in a holding pattern for now, and we can remove again once we are moving forward with a review. Okay with you?

@Robinlovelace
Copy link
Member Author

Robinlovelace commented Apr 13, 2015

Sound great @sckott. More detailed responses coming soon, but YES this package enables access to new open source data and YES it will be very well documented with vignettes etc. Check out my work on osmar, which is kind-of an introductory vignette to the package: https://github.com/Robinlovelace/osm-tutorial/blob/master/osm.pdf The reason I want to be affiliated with ropensci is that actively encourages such clear reproducibility, communication and empowerment through technology. Based on the supportive feedback holding sounds like the ideal solution for now.

@sckott
Copy link
Contributor

sckott commented Apr 14, 2015

@Robinlovelace Okay, sounds good. So this package already does data acquisition? Or it will? Do you mind if I uncheck boxes that aren't complete yet?

@Robinlovelace
Copy link
Member Author

Robinlovelace commented Apr 14, 2015

@sckott yes. gLines2CyclePath harvests data from the CycleStreet.net routing API. We plan to add interfaces to other routing APIs. Here are a couple of examples of the kind of data it pulls in, of a couple of routes in Manchester and a few thousand OD flows in Coventry:
selection_135

selection_179

@Robinlovelace
Copy link
Member Author

Robinlovelace commented Apr 14, 2015

Don't mind at all if you uncheck uncompleted boxes, please do.

@sckott
Copy link
Contributor

sckott commented Apr 14, 2015

@Robinlovelace Cool - nice example! Boxed unchecked.

@Robinlovelace
Copy link
Member Author

Robinlovelace commented Apr 16, 2015

@sckott OK, I'm down to only 3 warnings from 7.

Any ideas how to remove the remaining 3?

Also check the updated README:
https://github.com/Robinlovelace/stplanr

What else do I need in there for this to be 'ropensci compliant'?
Thanks for the support, more to come.

@sckott
Copy link
Contributor

sckott commented Apr 17, 2015

WARNING '::' or ':::' import not declared from: ‘sp’ 'library' or 'require' call not declared from: ‘sp’

  • Looks like you are using the sp package but you didn't put it into your DESCRIPTION file to load
  • Looking at your NAMESPACE file, I'd suggest not using exportPattern("^[[:alpha:]]+") and instead using roxygen @export to explicitly declare what functions you want exported to users, and which not exported - also, using @import tags (e.g., @import sp) will put the appropriate entry in your NAMESPACE file for packages to import

Namespaces in Imports field not imported from: ‘dplyr’ ‘RCurl’ ‘rgdal’ ‘rgeos’ All declared Imports should be used.

  • See above comment. Some of these packages are in your DESCRIPTION file, but some are not. R check found that you either A) use one of those packages in yours but it's not imported (in the DESCRIPTION and NAMESPACE files), or B) you don't use one of those packages in your package, but it's listed to import

WARNING LaTeX errors when creating PDF version. This typically indicates Rd problems.

  • Hard to say what's wrong here, other than that something with Rd files.

WARNING '::' or ':::' import not declared from: ‘sp’ 'library' or 'require' call not declared from: ‘sp’

  • You need to add sp to package imports

@sckott
Copy link
Contributor

sckott commented Apr 17, 2015

Also, I'm getting a bunch more warnings locally running check on your package. Do you not get these?

* using R version 3.1.3 (2015-03-09)
* using platform: x86_64-apple-darwin13.4.0 (64-bit)
* using session charset: UTF-8
* checking for file ‘stplanr/DESCRIPTION’ ... OK
* checking extension type ... Package
* this is package ‘stplanr’ version ‘0.0.0.9005’
* checking package namespace information ... OK
* checking package dependencies ... OK
* checking if this is a source package ... OK
* checking if there is a namespace ... OK
* checking for executable files ... OK
* checking for hidden files and directories ... OK
* checking for portable file names ... OK
* checking for sufficient/correct file permissions ... OK
* checking whether package ‘stplanr’ can be installed ... OK
* checking installed package size ... OK
* checking package directory ... OK
* checking DESCRIPTION meta-information ... NOTE
License components which are templates and need '+ file LICENSE':
  MIT
* checking top-level files ... OK
* checking for left-over files ... OK
* checking index information ... OK
* checking package subdirectories ... OK
* checking R files for non-ASCII characters ... OK
* checking R files for syntax errors ... OK
* checking whether the package can be loaded ... OK
* checking whether the package can be loaded with stated dependencies ... OK
* checking whether the package can be unloaded cleanly ... OK
* checking whether the namespace can be loaded with stated dependencies ... OK
* checking whether the namespace can be unloaded cleanly ... OK
* checking loading without being on the library search path ... OK
* checking dependencies in R code ... WARNING
'::' or ':::' import not declared from: ‘sp’
'library' or 'require' call not declared from: ‘sp’
'library' or 'require' call to ‘sp’ in package code.
  Please use :: or requireNamespace() instead.
  See section 'Suggested packages' in the 'Writing R Extensions' manual.
Namespace in Imports field not imported from: ‘dplyr’
  All declared Imports should be used.
* checking S3 generic/method consistency ... OK
* checking replacement functions ... OK
* checking foreign function calls ... OK
* checking R code for possible problems ... NOTE
disab_recat: no visible binding for global variable ‘b’
gFlow2line: no visible global function definition for ‘Line’
gFlow2line: no visible global function definition for ‘proj4string<-’
gFlow2line: no visible global function definition for ‘proj4string’
gLines2CyclePath: no visible global function definition for ‘Lines’
gLines2CyclePath: no visible global function definition for ‘Line’
gLines2CyclePath: no visible global function definition for
  ‘SpatialLines’
gLines2CyclePath: no visible global function definition for
  ‘proj4string<-’
gLines2CyclePath: no visible global function definition for ‘CRS’
gOverline: no visible global function definition for ‘over’
gOverline: no visible global function definition for
  ‘SpatialLinesDataFrame’
islines: no visible global function definition for ‘gIntersection’
lineLabels: no visible global function definition for ‘coordinates’
lineLabels: no visible global function definition for ‘gCentroid’
* checking Rd files ... OK
* checking Rd metadata ... OK
* checking Rd cross-references ... WARNING
Missing link or links in documentation object 'islines.Rd':
  ‘gOverline()’

See section 'Cross-references' in the 'Writing R Extensions' manual.

* checking for missing documentation entries ... WARNING
Undocumented code objects:
  ‘lineLabels’
All user-level objects in a package should have documentation entries.
See chapter ‘Writing R documentation files’ in the ‘Writing R
Extensions’ manual.
* checking for code/documentation mismatches ... OK
* checking Rd \usage sections ... WARNING
Undocumented arguments in documentation object 'gOverline'
  ‘fun’

Undocumented arguments in documentation object 'gSection'
  ‘sl’

Undocumented arguments in documentation object 'islines'
  ‘g1’ ‘g2’

Functions with \usage entries need to have the appropriate \alias
entries, and all their arguments documented.
The \usage entries must correspond to syntactically valid R code.
See chapter ‘Writing R documentation files’ in the ‘Writing R
Extensions’ manual.
* checking Rd contents ... OK
* checking for unstated dependencies in examples ... WARNING
'::' or ':::' import not declared from: ‘sp’
'library' or 'require' call not declared from: ‘sp’
* checking contents of ‘data’ directory ... OK
* checking data for non-ASCII characters ... OK
* checking data for ASCII and uncompressed saves ... OK
* checking examples ... ERROR
Running examples in ‘stplanr-Ex.R’ failed
The error most likely occurred in:

> ### Name: gOverline
> ### Title: Convert series of overlapping lines into a route network
> ### Aliases: gOverline
>
> ### ** Examples
>
> data(routes_fast)
> rnet <- gOverline(sldf = routes_fast[1:7,], attr = "length")
Error in gOverline(sldf = routes_fast

@Robinlovelace
Copy link
Member Author

Robinlovelace commented Apr 17, 2015

@sckott Yes, it's because I just added gOverline() and related functions which have caused additional warnings - there's been a lot of interest in the package of late and I expect more such functions, created by additional authors, to be added soon. Will debug based on your suggestions.

@Robinlovelace
Copy link
Member Author

Robinlovelace commented Apr 17, 2015

@sckott Good news: it's now down to just 2 warnings on my system after the latest commit:

WARNING
'library' or 'require' call not declared from: ‘sp’
See the information on DESCRIPTION files in the chapter ‘Creating R
packages’ of the ‘Writing R Extensions’ manual.

and

WARNING
LaTeX errors when creating PDF version.
This typically indicates Rd problems.

Not sure how to find exactly where the problems are emerging for either one. Any ideas?

Robin

@sckott
Copy link
Contributor

sckott commented Apr 17, 2015

@Robinlovelace still getting more errors than you are, what version of R are you using?

Also, looks like you are still using exportPattern("^[[:alpha:]]+")

@Robinlovelace
Copy link
Member Author

Robinlovelace commented Apr 18, 2015

@sckott I was using 3.1.2. Now upgraded to 3.2.0, had to change a few things to avoid errors. Looking at what I should put in the NAMESPACE now. Any pointers, other than the excellent resource on R Packages (Wickham 2015)? The latest log message can be found here: https://github.com/Robinlovelace/stplanr/blob/master/log-latest

Edit:

@export tags now used to generate the NAMESPACE.

@sckott
Copy link
Contributor

sckott commented Apr 19, 2015

@Robinlovelace nice, checked again with your most recent version and got no warnings

@Robinlovelace
Copy link
Member Author

Robinlovelace commented Apr 22, 2015

@sckott I now get no warnings also, thanks to this: http://stackoverflow.com/questions/29791500/how-to-identify-latex-errors-in-rd-r-help-files

Next stage: tests with testthat. Any advice for the tests? Can I use the packages own data to run the tests (e.g. cents in stplanr)?

Pointers for 'continuous integration' with Travis also appreciated.

@Robinlovelace
Copy link
Member Author

Robinlovelace commented Apr 22, 2015

I've also updated the first comment of this thread to reflect recent improvements in stplanr.

@karthik
Copy link
Member

karthik commented Apr 22, 2015

@Robinlovelace Do you have a vignette? If not, just ignore the latex warnings. Those are unrelated to the package itself.

@karthik
Copy link
Member

karthik commented Apr 22, 2015

Can I use the packages own data to run the tests (e.g. cents in stplanr)?

Yes!

Re Travis, I'd suggest starting here and we can help from there. http://r-pkgs.had.co.nz/check.html#travis

@jennybc
Copy link
Member

jennybc commented Apr 22, 2015

Re: using package's own data, here's a tiny example:

https://github.com/jennybc/gapminder/blob/master/tests/testthat/test-gapminder.R

gapminder is just a data package and has only this one test. But data/gapminder.rda is where the data lives within the package and I can just refer to the gapminder data.frame w/o further fuss in my tests. You should be able to do same with your data objects cents, flow, etc.

Re: Travis. If you are using devtools, there is a use_travis() function to set that process in motion. Note: The development version from GitHub has been updated to produce the new, simpler .travis.yml file, as illustrated in the link above. The released CRAN version produces an older, hairier file, FYI.

@Robinlovelace
Copy link
Member Author

Robinlovelace commented May 8, 2015

Thanks for the information @jennybc, I've now added my first unit test (for gFlow2Line) and it seems to work! I will do the same for the datasets if needs be, but need to prioritise tests for the most important things...

@sckott I'm on the R Packages course taught by Colin Gillespie in Newcastle now. I've added continuous testing with Travis and (after installing r-gdal from the system, not R) it builds successfully! I've also added a vignette and a demo.

Next step that should make the package even more attractive from an rOpenSci perspective is the addition of tools to automatically download and query OSM data: empowering people via data access. When this is implemented, can we move forward by assigning an editor etc?

Finally, by adding a blank line in demo/00Index (randomly!) and adding sp as a dependency (it really is) I've just eliminated all warnings on my system. Wahey!

@sckott
Copy link
Contributor

sckott commented May 8, 2015

Nice progress! yes, adding OSM data access would fit better into our wheel-house

@sckott sckott added the package label Jun 29, 2015
@Robinlovelace
Copy link
Member Author

Robinlovelace commented Oct 12, 2015

Hi @sckott I think we're ready to move this forward again towards peer review. @richardellison has added functionality for reading in data from official Australian sources and the addition of route_graphhopper() means that we can now use this to access routed osm data from anywhere in the world. E.g. (once you have the graphhopper api key):

#' @examples
#'
#' \dontrun{
#' r <- route_graphhopper("Leeds", "Dublin", vehicle = "bike")
#'
#' library(leaflet)
#'
#' leaflet() %>% addTiles() %>% addPolylines(data = r)
#' }

leeds-route

@sckott
Copy link
Contributor

sckott commented Oct 12, 2015

Cool. So you want to start review?

@Robinlovelace
Copy link
Member Author

Robinlovelace commented Oct 18, 2015

Thanks @sckott for this detailed review. I've pushed a load of changes that fix some of the 'quick win' issues. I'll repeat your comments here and strike-through, with comments after, as they get fixed.

  • Use @importFrom whenever possible. Right now you have import(openxlsx) and import(sp) in your NAMESPACE file. Just import the functions you need. Same for other pkg deps.

Please take a look at my new package description stplanr-package.R: seem OK?

  • On check I saw Rd file 'route_cyclestreet.Rd' - make sure line widths for roxygen docs aren't more than 100 characters.

Should be fixed by Robinlovelace/stplanr@6271997

  • It appears that XML package is not used at all? If not, remove from DESCRIPTION file.

True - was a relic from my attempts to parse .osm files: now fixed.

  • Tests: Pleae write tests to cover at least all the major functions before we accept. Use testthat::skip_on_cran() for any tests that do web requests, so that CRAN tests don't fail in case a service is temporarily down

This is great advice re. robust tests: I will ask for a public API key for testing the routing data. One problem is that ggmap::geocode seems unreliable so make look for a replacement. This comment is the biggy for me so any suggestions, let me know.

  • Title in DESCRIPTION file should be sentence case

I think this fixes the issue: Robinlovelace/stplanr@f4e28a9 please let me know.

Fixed with Robinlovelace/stplanr@2fff807

  • Curious whether you really need sp in Depends, or if you could have in Imports.

Yes this has come up before. I tried to remove it but many things broke: I really think sp is a valid dependency here: a small package that is essential for handling the spatial data classes used by stplanr.

  • README: At bottom of readme, you say to contact you via email. That's cool, but also direct to the Issue tracker in the repo

Fixed here: Robinlovelace/stplanr@e780258

  • Point to instructions in readme for installing rgdal and rgeos, they are often painful for folks

Fixed here for Ubuntu - suspect it's less of an issue for Windows users but I don't know how to fix this for Mac users: Robinlovelace/stplanr@84aad65

  • I think token's can be a bit easier for the user. Right now you have e.g.,

Fixed based on advice from here: https://cran.r-project.org/web/packages/httr/vignettes/api-packages.html

Fix: Robinlovelace/stplanr@a6d8927

  • gOverline for me makes R crash. e.g., this gOverline(routes_fast[c(2, 3, 22),], attrib = "length"). Does that work for you?

Yes it does - not sure what's up with that. Please see image below. What's your session_info?

selection_011

  • Throughtout package, make sure to have examples that will work for anyone.

Agreed - we're on the case!
I think all the examples now work, e.g. after: Robinlovelace/stplanr@fdc02d1

  • Looks like for web requests you're doing are directly through jsonlite::fromJSON(). We strongly urge you to use httr to do web requests.

This seems like very good advice. I had some strange behaviour from jsonlite so that should help.

Done - please see here: Robinlovelace/stplanr@762ea2e

  • Use a code of conduct, see devtools::use_code_of_conduct()

Job done: Robinlovelace/stplanr@03272dc

So do I and I'm not following my own advice here! Will move to consistent snake_case.

Even though I think gOverline() is a cool name ;) What do you say @barryrowlingson?

Please see fix here: Robinlovelace/stplanr@815afa2

@sckott
Copy link
Contributor

sckott commented Oct 19, 2015

A bit more review:

  • The route_cyclestreet() examples failed for me. The ggmap::geocode calls don't work:

    route_cyclestreet(from = "Hereford", to = "Leeds", plan = "quietest")
    #> Information from URL : http://www.datasciencetoolkit.org/maps/api/geocode/json?address=Hereford&sensor=false
    #> Information from URL : http://www.datasciencetoolkit.org/maps/api/geocode/json?address=Leeds&sensor=false
    #> 1: In readLines(connect, warn = FALSE) :
    #>   cannot open: HTTP status was '0 (null)'
    #> 2: In ggmap::geocode(from) :   geocoding failed for "Hereford".
    #>   if accompanied by 500 Internal Server Error with using dsk, try google.
    #> 3: In readLines(connect, warn = FALSE) :
    #>   cannot open: HTTP status was '0 (null)'
    #> 4: In ggmap::geocode(to) :   geocoding failed for "Leeds".
    #>   if accompanied by 500 Internal Server Error with using dsk, try google.

    Is there something simple I'm missing. Do I need auth to use ggmap::geocode. Perhaps you could use geonames package? E.g.,

    geonames::GNpostalCodeSearch(postalcode="M3 4EE")
    #>   adminCode2 adminCode3              adminName3 adminCode1               lng countryCode postalCode adminName1        placeName
    #> 1             E08000003 Manchester District (B)        ENG -2.24808567051586          GB     M3 4EE    England City Centre Ward
    #>           lat
    #> 1 53.4772316872306
  • Same for route_graphhopper()

@sckott
Copy link
Contributor

sckott commented Oct 19, 2015

Please take a look at my new package description stplanr-package.R: seem OK?

Yep

One problem is that ggmap::geocode seems unreliable so make look for a replacement. This comment is the biggy for me so any suggestions, let me know.

See my above comment, perhaps geonames

Point to instructions in readme for installing rgdal and rgeos, they are often painful for folks
Fixed here for Ubuntu - suspect it's less of an issue for Windows users but I don't know how to fix this for Mac users

e.g, for macs https://github.com/ropensci/geojsonio#install

Any advice re. how to prompt? Should it be a warning that it needs api keys on package load or only on the function calls?

Not on package load. You could also have a separate function to set keys, a wrapper around Sys.setenv

gOverline crashing: Please see image below. What's your session_info?

after loading stplanr

```r
Session info ---------------------------------
 setting  value                                      
 version  R version 3.2.2 Patched (2015-08-14 r69078)
 system   x86_64, darwin13.4.0                       
 ui       RStudio (0.99.703)                         
 language (EN)                                       
 collate  en_US.UTF-8                                
 tz       America/Los_Angeles                        
 date     2015-10-19                                 

Packages --------------------------------------------- 
package  * version date       source                                
 devtools * 1.9.1   2015-09-11 CRAN (R 3.2.0)                        
 digest     0.6.8   2014-12-31 CRAN (R 3.2.0)                        
 lattice    0.20-33 2015-07-14 CRAN (R 3.2.2)                        
 memoise    0.2.1   2014-04-22 CRAN (R 3.2.0)                        
 openxlsx   3.0.0   2015-07-03 CRAN (R 3.2.0)                        
 Rcpp       0.12.1  2015-09-10 CRAN (R 3.2.2)                        
 sp       * 1.2-0   2015-09-08 CRAN (R 3.2.2)                        
 stplanr  * 0.0.1.0 2015-10-16 Github (Robinlovelace/stplanr@0dac505)
```

@Robinlovelace
Copy link
Member Author

Robinlovelace commented Oct 22, 2015

Hi @sckott to provide a quick-fire answer to your question "Perhaps you could use geonames package?" I tested it but found it less effective that google's place search algorithm at finding places with 'fuzzy names' such as "Chapeltown, Leeds, UK". I've replaced ggmap with RgoogleMaps for now as the former seems buggy.

@Robinlovelace
Copy link
Member Author

Robinlovelace commented Oct 23, 2015

I think all the outstanding issues except for the unit tests for all functions and the issue with gOverline (now called overline) have now been tackled. Please can you reproduce on other computers - I've tested it and it seems to work.. I've updated the original response message with more cross-outs and comments (see above). I added unit tests for more functions in a commit today, but there are still functions that lack tests. The issues with ggmap have been resolved by use of RgoogleMaps::getGeoCode(). Thanks to your input, R now asks you for your api key interactively if its not set when you use the route_ functions. Please give them a spin!

Many thanks for the review so far.

@sckott
Copy link
Contributor

sckott commented Oct 24, 2015

I tested it but found it less effective

okay, thanks for checking

I'll give it a spin...

@sckott
Copy link
Contributor

sckott commented Oct 26, 2015

@Robinlovelace Tested the routes functions that require keys. Works nicely now!

@Robinlovelace
Copy link
Member Author

Robinlovelace commented Oct 27, 2015

Thanks @sckott, really grateful for your suggestions on improving them. I hope to add more routing apis in due course. Perhaps eventually route() could become generic, pulling in route_cyclestreet, route_graphhopper and route_osrm depending on settings.

@sckott
Copy link
Contributor

sckott commented Oct 27, 2015

Sure, and nice work on the package.

Nothing major left I can think of, but installing with building vignette for me turns up an error NOTE: rgdal::checkCRSArgs: no proj_defs.dat in PROJ.4 shared files - which makes the install fail. Do you ever get this error?

@Robinlovelace
Copy link
Member Author

Robinlovelace commented Oct 27, 2015

Hi Scott, no I don't get that error message. Following advice from here, I get:

devtools::install_github("robinlovelace/stplanr", build_vignettes = T)
Downloading GitHub repo robinlovelace/stplanr@master
## lots of messages associated with install...
## ...
* DONE (stplanr)
Reloading installed stplanr

The only strange thing was this note but I don't think that's anything to do with stplanr:

Note: the specification for S3 class “AsIs” in package ‘jsonlite’ seems equivalent to one from package ‘RJSONIO’: not turning on duplicate class definitions for this class.

Regarding the ROpenSci process, what's the next stage? Do you recommend I submit it to CRAN?

@sckott
Copy link
Contributor

sckott commented Oct 27, 2015

Nevermind, had to fix my proj installation - there was a problem in most recent proj http://r-sig-geo.2731867.n2.nabble.com/rgdal-1-0-7-td7588869.html

@sckott
Copy link
Contributor

sckott commented Oct 27, 2015

I am getting a segfault though on the call to overline() https://github.com/Robinlovelace/stplanr/blame/master/vignettes/introducing-stplanr.Rmd#L202 Any thoughts?

@Robinlovelace
Copy link
Member Author

Robinlovelace commented Oct 27, 2015

I don't get any error on my Linux laptop. Just tried on my Windows work desktop: no issues either.

@sckott
Copy link
Contributor

sckott commented Oct 27, 2015

What version of GDAL/GEOS do you have? I have gdal 1.11.1, geos 3.4.2, proj 4.9.2

@Robinlovelace
Copy link
Member Author

Robinlovelace commented Oct 27, 2015

I get

> rgdal::getGDALVersionInfo()
[1] "GDAL 1.11.2, released 2015/02/10"

and

> rgeos::version_GEOS()
[1] "3.4.2-CAPI-1.8.2 r3921"

on both Windows and Linux systems.

@sckott
Copy link
Contributor

sckott commented Oct 27, 2015

Thanks. Other people can't reproduce this bug, so we'll figure it out later if it persists, looks to be just osx, and just me so far :)

Just one more thing on a check I just ran, a few lines too long

Rd file 'calc_catchment.Rd':
  \usage lines wider than 90 characters:
       projection = "+proj=aea +lat_1=90 +lat_2=-18.416667 +lat_0=0 +lon_0=10 +x_0=0 +y_0=0 +ellps=GRS80 +towgs84=0,0,0,0,0,0,0 +units=m +no ... [TRUNCATED]

Rd file 'calc_catchment_sum.Rd':
  \usage lines wider than 90 characters:
       projection = "+proj=aea +lat_1=90 +lat_2=-18.416667 +lat_0=0 +lon_0=10 +x_0=0 +y_0=0 +ellps=GRS80 +towgs84=0,0,0,0,0,0,0 +units=m +no ... [TRUNCATED]

@sckott
Copy link
Contributor

sckott commented Oct 27, 2015

After that, we're ready to bring it in to ropensci.

@Robinlovelace
Copy link
Member Author

Robinlovelace commented Oct 27, 2015

Great. This fixes the 90 char limit note - suggest we're ready to go!
Robinlovelace/stplanr@7a2cfdd

@Robinlovelace
Copy link
Member Author

Robinlovelace commented Oct 27, 2015

Planning to submit this to CRAN later this week. If you or anyone has any advice on that, please let me know. Many thanks for this review - it's been great having all out in the open and makes me wonder what benefits open peer review could have for academic publications. I guess ROpenSci will fork this now and I should push to this fork from now on?

@sckott
Copy link
Contributor

sckott commented Oct 27, 2015

I guess ROpenSci will fork this now and I should push to this fork from now on?

I made a team within ropensci org account, you should be able to transfer the repo to ropensci after you accept that invitation (i think)

@Robinlovelace
Copy link
Member Author

Robinlovelace commented Oct 27, 2015

Hi @sckott thanks for inviting me to @ropensci. I've accepted but cannot transfer stplanr. I think this is because don't have the right permissions, based on this: I cannot create new repos it seems. Maybe you need to give me more permissions? I had some reservations about transferring stplanr to ropensci but having checked out more about the organisation I think it's the right thing to do: encourages community participation and provides some visibility. The support you guys provide is fantastic and it's soooo good having someone review your code: awesome work on bringing open access peer review into R package development!

@sckott
Copy link
Contributor

sckott commented Oct 28, 2015

Thanks, we think reviewing is going pretty well :)

Did you go to Settings > Transfer Ownership (near the bottom) ? That didn't work?

@Robinlovelace
Copy link
Member Author

Robinlovelace commented Oct 28, 2015

Correct:
selection_024

@sckott
Copy link
Contributor

sckott commented Oct 28, 2015

try again after refreshing...

@sckott
Copy link
Contributor

sckott commented Oct 28, 2015

if that doesn't work, give me admin on your repo, then I think I should be able to transfer

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

5 participants