Closed mem48 closed 6 years ago
P.S. I haven't rebuilt the descriptions etc to avoid merge conflicts
Any thoughts on this @marcusyoung will be good to get it rolling.
I didn't get a notification from Github about this. I'll take a look.
@marcusyoung I've switched to make_url() and also made a load more improvements.
I've made some general updates to make it more "package friendly". Adding some vignettes based on your tutorial and sample data. I've also updated the description and documentation so devtoolls::install_github()
works.
This PR is getting huge! Looks good, suggest merging soon. Can always tidy-up details after the event.
Biggest problem from CRAN perspective: giant size. 30 MB+. Suggest using the piggyback package or similar to help separate code and data:
git clone git@github.com:mem48/opentripplanner-malcolm
Cloning into 'opentripplanner-malcolm'...
remote: Counting objects: 291, done.
remote: Compressing objects: 100% (147/147), done.
remote: Total 291 (delta 114), reused 172 (delta 66), pack-reused 70
Receiving objects: 100% (291/291), 34.04 MiB | 5.79 MiB/s, done.
Resolving deltas: 100% (131/131), done.
The large size is the sample data, I'd been copying @marcusyoung tutorial.
I slimmed down his original data a bit but still
DEM.tif 5 MB greater-manchester-osm.pbf 11 MB tfgm-gtfs.zip 33 MB
Perhaps we should build a new tutorial on a smaller location, e.g. the Isle of White. The problem then will be GTFS data which is waiting on my https://github.com/mem48/UK2GTFS package
@mem48 @Robinlovelace Apologies for delay with this. Now reviewing. I'll comment as I go, but am not saying these issues all need addressing before I merge. As Robin said these can be tidied up later if needs be. One general thing is there's a lot in this pull request. Probably be best to have a separate branch per function/group of related functions in future if there aren't dependencies between? Makes it easier to review and quicker to merge as an issue in one area won't hold up those ready to go.
@mem48 Just finished reviewing otp-setup.R. Some great stuff in there. I've made a few comments, see above. Quite a few typos in the help content, not worrying about that at the moment, but will need checking thoroughly at some point.
@mem48 I've reviewed otp-route. Comments on specific bits of code above. Basic routing by car/walk/cycle seems to be working great. There's a problem requesting TRANSIT/BUS/TRAM/RAIL routes as I've mentioned above that needs looking into. I think it is being caused by the date format YYYY-MM-DD instead of MM-DD-YYYY required by OTP. Lots of typos to look at, including in user-facing stuff - help and error messages etc.
@mem48 Taken a look at otp-geocode. The type argument is missing from the function definition so unable to test.
@mem48 Have taken a look at otp-isochrone. This has similar problem to otp-route for TRANSIT modes when specifiying date and time, I think because the date format is not right for OTP. Currently returns empty polygons in this situation, so also need to look at how we pick up errors.
Function currently returns only an SF object. I think going forward the return format should be optional, additionally including GEOJSON (and possibly SHAPEFILE) Both are the native options available from OTP. Users will not always want to continue processing in R, e.g. writing direct to PostgreSQL.
@mem48 Have had a look at the new Vignettes. Great work. A few comments above.
@mem48 Looks like we've got some internal functions appearing in the man folder.
Replaced by pr17
A new file otp-setup.R containing 4 new functions
otp_build_graph - builds an OPT graph otp_setup - starts a local OPT server otp_stop - stops a local OPT server otp_checks - runs some basic checks, do we have the correct version of Java installed etc. Called internally by other above functions
The idea is that people who are R users may not be familiar with Java and/or command line so we provide these functions to simplify setting up the OTP.
Currently only tested on Windows 10, should work on other windows version but some functions like otp_stop will not work on other OS.
Simple example: