-
-
Notifications
You must be signed in to change notification settings - Fork 104
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
Comments
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? |
@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 |
@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.
cool, sounds good! The
|
@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 |
@Robinlovelace let's leave this issue open for now. I added the |
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 |
@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? |
@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: |
Don't mind at all if you uncheck uncompleted boxes, please do. |
@Robinlovelace Cool - nice example! Boxed unchecked. |
@sckott OK, I'm down to only 3 warnings from 7. Any ideas how to remove the remaining 3? Also check the updated README: What else do I need in there for this to be 'ropensci compliant'? |
|
Also, I'm getting a bunch more warnings locally running check on your package. Do you not get these?
|
@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. |
@sckott Good news: it's now down to just 2 warnings on my system after the latest commit:
and
Not sure how to find exactly where the problems are emerging for either one. Any ideas? Robin |
@Robinlovelace still getting more errors than you are, what version of R are you using? Also, looks like you are still using |
@sckott I was using Edit:
|
@Robinlovelace nice, checked again with your most recent version and got no warnings |
@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. Pointers for 'continuous integration' with Travis also appreciated. |
I've also updated the first comment of this thread to reflect recent improvements in stplanr. |
@Robinlovelace Do you have a vignette? If not, just ignore the latex warnings. Those are unrelated to the package itself. |
Yes! Re Travis, I'd suggest starting here and we can help from there. http://r-pkgs.had.co.nz/check.html#travis |
Re: using package's own data, here's a tiny example: https://github.com/jennybc/gapminder/blob/master/tests/testthat/test-gapminder.R
Re: Travis. If you are using |
Thanks for the information @jennybc, I've now added my first unit test (for @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 |
Nice progress! yes, adding OSM data access would fit better into our wheel-house |
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
|
Cool. So you want to start review? |
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.
Please take a look at my new package description stplanr-package.R: seem OK?
Should be fixed by Robinlovelace/stplanr@6271997
True - was a relic from my attempts to parse .osm files: now fixed.
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.
I think this fixes the issue: Robinlovelace/stplanr@f4e28a9 please let me know.
Fixed with Robinlovelace/stplanr@2fff807
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.
Fixed here: Robinlovelace/stplanr@e780258
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
Fixed based on advice from here: https://cran.r-project.org/web/packages/httr/vignettes/api-packages.html Fix: Robinlovelace/stplanr@a6d8927
Yes it does - not sure what's up with that. Please see image below. What's your session_info?
Done - please see here: Robinlovelace/stplanr@762ea2e
Job done: Robinlovelace/stplanr@03272dc
Please see fix here: Robinlovelace/stplanr@815afa2 |
A bit more review:
|
Yep
See my above comment, perhaps
e.g, for macs https://github.com/ropensci/geojsonio#install
Not on package load. You could also have a separate function to set keys, a wrapper around
after loading
|
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. |
I think all the outstanding issues except for the unit tests for all functions and the issue with Many thanks for the review so far. |
okay, thanks for checking I'll give it a spin... |
@Robinlovelace Tested the routes functions that require keys. Works nicely now! |
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. |
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 |
Hi Scott, no I don't get that error message. Following advice from here, I get:
The only strange thing was this note but I don't think that's anything to do with stplanr:
Regarding the ROpenSci process, what's the next stage? Do you recommend I submit it to CRAN? |
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 |
I am getting a segfault though on the call to |
I don't get any error on my Linux laptop. Just tried on my Windows work desktop: no issues either. |
What version of GDAL/GEOS do you have? I have gdal 1.11.1, geos 3.4.2, proj 4.9.2 |
I get
and
on both Windows and Linux systems. |
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] |
After that, we're ready to bring it in to ropensci. |
Great. This fixes the 90 char limit note - suggest we're ready to go! |
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? |
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) |
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! |
Thanks, we think reviewing is going pretty well :) Did you go to Settings > Transfer Ownership (near the bottom) ? That didn't work? |
try again after refreshing... |
if that doesn't work, give me admin on your repo, then I think I should be able to transfer |
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).
https://github.com/Robinlovelace/stplanr
Example data on route allocation and travel flows (included - more planned), geographical boundaries, transport networks, example travel survey data (to be added subsequently)
Transport researchers and modellers from academic, public and private sectors. University students studying transport geography, transport modelling and related disciplines.
devtools::check()
produce any errors or warnings? If so paste them below.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.
The text was updated successfully, but these errors were encountered: