Submission: opentripplanner

[mem48]

Submitting Author: Malcolm Morgan (@mem48)
Repository: https://github.com/ITSLeeds/opentripplanner
Version submitted: 0.1.0
Editor: @geanders
Reviewer 1: TBD
Reviewer 2: TBD
Archive: TBD
Version accepted: TBD

---

• Paste the full DESCRIPTION file inside a code block below:

Package: opentripplanner
Title: OpenTripPlanner for R
Version: 0.1.0.0000
Authors@R: c(
    person("Marcus", "Young",   email = "M.A.Young@soton.ac.uk", role = c("aut"),
      comment = c(ORCID = "0000-0003-4627-1116")),
    person("Malcolm", "Morgan", email = "m.morgan1@leeds.ac.uk", role = c("aut","cre"), 
      comment = c(ORCID = "0000-0002-9488-9183")),
    person("Robin", "Lovelace", email = "rob00x@gmail.com",      role = "aut",
      comment = c(ORCID = "0000-0001-5679-6536")),
    person("Layik", "Hama",     email = "layik.hama@gmail.com",  role = "ctb",
      comment = c(ORCID = "0000-0003-1912-4890"))
    )
Maintainer: Malcolm Morgan <m.morgan1@leeds.ac.uk>
Description: An R interface for OpenTripPlanner <http://www.opentripplanner.org/>.
    OpenTripPlanner (OTP) is an open source platform for multi-modal and multi-agency
    journey planning written in Java. OTP provides a REST API which OpenTripPlanner
    for R uses to connect to OTP.
Language: EN-GB
License: file LICENSE
URL: https://github.com/ITSLeeds/opentripplanner, https://itsleeds.github.io/opentripplanner/
BugReports: https://github.com/ITSLeeds/opentripplanner/issues
Encoding: UTF-8
LazyData: true
Imports:
    checkmate,
    httr,
    sf,
    googlePolylines,
    dplyr,
    geodist,
    jsonlite,
    pbapply
RoxygenNote: 6.1.1
Suggests: 
    testthat,
    knitr,
    rmarkdown,
    covr
VignetteBuilder: knitr

Scope

• Please indicate which category or categories from our package fit policies this package falls under: (Please check an appropriate box below. If you are unsure, we suggest you make a pre-submission inquiry.):

  • data retrieval
  • data extraction
  • database access
  • data munging
  • data deposition
  • reproducibility
  • geospatial data
  • text analysis

• Explain how and why the package falls under these categories (briefly, 1-2 sentences):
  Open Trip Planner is a multimodal route planning tool written in Java, the package allows R users to access the functionality of Open Trip Planner running locally or on a server. So, the package performs the role of data retrieval of geospatial data.

• Who is the target audience and what are scientific applications of this package?
  Transport planners and people engaging in transport-related research. Route planning is the basis of many different transport related research questions such as traffic management, accessibility measurement, and infrastructure planning. The package is being actively developed to support research at the University of Leeds Institute for Transport Studies.

• Are there other R packages that accomplish the same thing? If so, how does yours differ or meet our criteria for best-in-category?
  There are many packages that enable route planning in R (e.g. osrmr & dodgr) However there are no packages on CRAN that connect R to the Open Trip Planner. We are aware of one package on GitHub (https://github.com/datasciencecampus/access-to-services/tree/develop/propeR ) which relates to connecting R and OTP although its purpose is more specialised.
  A key benefit of OTP over other route planning tools is its integration of public transport and other modes of travel (e.g. bike hire) that are not supported by other tools such as osrm.

• If you made a pre-submission enquiry, please paste the link to the corresponding issue, forum post, or other discussion, or @tag the editor you contacted.

Technical checks

Confirm each of the following by checking the box. This package:

• does not violate the Terms of Service of any service it interacts with.
• has a CRAN and OSI accepted license.
• contains a README with instructions for installing the development version.
• includes documentation with examples for all functions.
• contains a vignette with examples of its essential functions and uses.
• has a test suite.
• has continuous integration, including reporting of test coverage using services such as Travis CI, Coveralls and/or CodeCov.

Publication options

• Do you intend for this package to go on CRAN?
• Do you wish to automatically submit to the Journal of Open Source Software? If so:

JOSS Options

• The package has an obvious research application according to JOSS's definition.
  • The package contains a paper.md matching JOSS's requirements with a high-level description in the package root or in inst/.
  • The package is deposited in a long-term repository with the DOI:
  • (Do not submit your package separately to JOSS)

• Do you wish to submit an Applications Article about your package to Methods in Ecology and Evolution? If so:

MEE Options

• The package is novel and will be of interest to the broad readership of the journal.
• The manuscript describing the package is no longer than 3000 words.
• You intend to archive the code for the package in a long-term repository which meets the requirements of the journal (see MEE's Policy on Publishing Code)
• (Scope: Do consider MEE's Aims and Scope for your manuscript. We make no guarantee that your manuscript will be within MEE scope.)
• (Although not required, we strongly recommend having a full manuscript prepared when you submit here.)
• (Please do not submit your package separately to Methods in Ecology and Evolution)

Code of conduct

• I agree to abide by rOpenSci's Code of Conduct during the review process and in maintaining my package should it be accepted.

[mem48]

Submission after #291

[melvidoni]

Hello @mem48! Thanks for the submission. @geanders has been assigned as your editor.

[mem48]

Thanks, @melvidoni and @geanders looking forward to your feedback.

[geanders]

@mem48 : So sorry for the delay in the initial checks from my end! This looks like a very exciting package, and I'm looking forward to managing the submission. I'll start with some results and comments from some standard pre-review checks.

Editor checks:

• Fit: The package meets criteria for fit and overlap
• Automated tests: Package has a testing suite and is tested via Travis-CI or another CI service.
• License: The package has a CRAN or OSI accepted license
• Repository: The repository link resolves correctly
• Archive (JOSS only, may be post-review): The repository DOI resolves correctly
• Version (JOSS only, may be post-review): Does the release version given match the GitHub release (v1.0.0)?

---

Editor comments

From spelling::spell_check_package:

All seem fine. There don't seem to be any legitimate typos caught by spell checking.

From unit tests:

First, as a note to reviewers, you'll need a system variable named "I_have_OTP" with the value "TRUE" if you want to run all the test (otherwise, a number are skipped). You can create this from R with the call:

Sys.setenv(I_have_OTP = "TRUE")

(@mem48 : You may want to consider changing this to use a "NOT_CRAN" system variable instead, as this seems to be used more frequently for these types of checks of whether you're using a local system or not.)

I am consistently getting an error for one of the tests:

test_03_with_OTP.R:186: failure: otp_stop
grepl("SUCCESS", foo) isn't true.

Do you have any thoughts on whether this is a true error, or if I've neglected something needed for the package in setting up my system? I'm thinking likely the latter, as your Travis CI build is passing. In either case, any tips on resolving this on my system would be helpful, as I need to resolve this error to run a cleaner test of the testing coverage of the package.

From goodpractice::gp():

It is good practice to

  ✖ avoid long code lines, it is bad for readability. Also, many people prefer editor windows that are about 80
    characters wide. Try make your lines shorter than 80 characters

    R/otp-config.R:84:1
    R/otp-config.R:85:1
    R/otp-config.R:89:1
    R/otp-config.R:90:1
    R/otp-config.R:91:1
    ... and 131 more lines

  ✖ avoid sapply(), it is not type safe. It might return a vector, or a list, depending on the input data.
    Consider using vapply() instead.

    R/otp-plan.R:214:20
    R/otp-plan.R:230:13

  ✖ avoid the library() and require() functions, they change the global search path. If you need to use other
    packages, import them. If you need to load them explicitly, then consider loadNamespace() instead, or as a last
    resort, declare them as 'Depends' dependencies.

    R/otp-plan.R:165:7

  ✖ checking tests ... ERROR Running the tests in ‘tests/testthat.R’ failed. Last 13 lines of output: 6188 ??
    0:00.00 grep java ── 1. Failure: otp_stop (@test_03_with_OTP.R#186) 
    ────────────────────────────────────────────────────────────────────────────────────── grepl("SUCCESS", foo)
    isn't true. ══ testthat results
    ════════════════════════════════════════════════════════════════════════════════════════════════════════════════════
    OK: 92 SKIPPED: 0 FAILED: 1 1. Failure: otp_stop (@test_03_with_OTP.R#186) Error: testthat unit tests failed In
    addition: Warning messages: 1: In getClassDef(class1) : closing unused connection 4 (<-localhost:11403) 2: In
    getClassDef(class1) : closing unused connection 3 (<-localhost:11403) Execution halted
  ✖ avoid 'T' and 'F', as they are just variables which are set to the logicals 'TRUE' and 'FALSE' by default,
    but are not reserved words and hence can be overwritten by the user.  Hence, one should always use 'TRUE' and 'FALSE'
    for the logicals.

    R/otp-plan.R:NA:NA

Based on these outputs, could you please:

• Shorten code lines longer than 80 characters
• Change uses of sapply to vapply or another safer alternative if possible
• Take out the use of library or require in the "opt-plan.R" file if possible (or explain in this thread why it needs to be kept---I see it's calling the current package, opentripplanner, for a parallel implementation, so maybe it's hard to work around using library for that?)
• Fix the "otp_stop" unit test (if it's a general problem, not just something I've missed in the set-up for my system)
• Change use of "T" and "F" to "TRUE" and "FALSE"

Documentation

Really nice job on the extensive online documentation for this package! I had a few comments for you to consider on the documentation at this stage

• You refer to "GTFS" a few times, but I didn't see it defined. Will this acronym be clear to most of your readers? If not, could you define it in the documentation?
• Is the geographical coverage of OpenTripPlanner worldwide? If so, could you specify that in the "getting started" document, or if not, clarify the geographical coverage somewhere?
• For the "prerequisites", I think your link to download Java if someone doesn't already have it might need to be fixed. I downloaded Java for my system from this link and installed it, but then got a message when I tried to run the package code that I needed "JDK", so I ended up installing that directly from Oracle's Java page and then everything ran successfully. You may want to doublecheck where you send people to download Java (and maybe specify that they'll need JDK, if that is indeed the case).

You can go ahead and work on these if you'd like, but none are egregious (the testing issue would be the most time-sensitive to get done), so I'm comfortable starting to look for reviewers for the package now. I'll update this comment with the names of the reviewers and their due date once I find them. Please let me know throughout the process if you have any questions!

---

Reviewers:
Reviewer #1: @kimnewzealand
Reviewer #2: @mifioriti

Due date: July 2, 2019

[mem48]

@geanders Thanks for the feedback. I'm trying to reproduce the error you are getting with the tests. Otherwise, I think these are all easy fixes.

[geanders]

@mem48 : That sounds good. If you are unable to reproduce the test error I got, then I think we'd be okay with seeing if it comes up for the reviewers.

[Robinlovelace]

Just looking at this. Please could you provide info on your system @geanders ? Suspect it may be an OS-specific issue. I'm on Ubuntu and also fails. otp_stop() is interactive, and asks:

otp_stop()
This will force Java to close, Press [enter] to continue, [escape] to abort
The following Java instances have been found:
 3659 pts/0    00:00:19 java

[geanders]

That seems plausible, @Robinlovelace. I'm using MacOS Mojave 10.14.4.

[Robinlovelace]

Think I've fixed that issue now.

[mem48]

@geanders I've been chipping away at the to-do list. I think they are all done now, with a few exceptions

• There are a few 80+ character lines left where long character strings such as URLs can't easily be split
• I haven't changed the I_have_OTP for now as NOT_CRAN is similar but not exactly the same. I'm open to advice on this.

I think the otp_stop test is fixed but I don't have a mac so perhaps you or @layik could confirm?

Thanks for all your help.

[geanders]

@mem48 : That sounds great. We've not got one assigned reviewer(@kimnewzealand) and I have a request out for our second reviewer.

[layik]

@mem48 I stumbled upon this by chance. GitHub might just be too good or I am not getting proper feeds. I will try it when I can. Just RE

There are a few 80+ character lines left where long character strings such as URLs can't easily be split

I will see if I can split them, R's paste0 is my friend when it comes to that, you might as well use part of those long URLs, if not it will satisfy roxygen or whatever package it is.

[kimnewzealand]

@mem48 : That sounds great. We've not got one assigned reviewer(@kimnewzealand) and I have a request out for our second reviewer.

super excited to help out

[mem48]

@layik thanks for the help. On the 80+ characters I know you can split and paste together again, but I'm not sure if that is good practice? My impression was that that the 80 character limit was advice rather than a strict limit.

[kimnewzealand]

I have raised an issue (see here ropensci/opentripplanner#28)

[kimnewzealand]

Thanks for the help with the issue #28. That can probably be closed?

I have raised another issue ( see here ropensci/opentripplanner#30)

[geanders]

@kimnewzealand : Thanks so much for this feedback on the package! If you wouldn't mind, would you please copy these issues here in this thread? If possible, we like to try to have a full copy of the discussion during the review process in one place, so we have a record of it that others can easily reference in the future in one place.

[kimnewzealand]

I have raised an issue (see here ITSLeeds/opentripplanner#28)

@kimnewzealand opened this issue 2 days ago · 4 comments

kimnewzealand commented 2 days ago •
Hi there opentripplanner team

I am going through the installation and set up for the package review. I have updated Java to version 8 and installed and attached the R package opentripplanner, as per the vignettes. I am raising this is as an issue as I am unable to progress on the review at this point.

I initially suspected there an issue with the OTP connection as the check for OTP did not return TRUE. (I did not change the .Renviron file at that point, as suggested in the vignette, see next).
Sys.getenv("I_have_OTP")

[1] ""

The build of a graph object also returned an error message:

log <- otp_build_graph(otp = path_otp, dir = path_data)

Basic checks completed, building graph, this may take a few minutes
Failed to build graph with message:
Error occurred during initialization of VMCould not reserve enough space for 2097152KB object heap
Warning message:
In system(text, intern = TRUE) :
running command 'java -Xmx2G -jar > >"C:\Users\Home\AppData\Local\Temp\RtmpoNFxd9/OTP/otp.jar" --build >"C:\Users\Home\AppData\Local\Temp\RtmpoNFxd9/OTP/graphs/default"' had status 1

I did some Googling and tried this fix on the environment variables in the control panel:
System Variables->New:
Variable name: _JAVA_OPTIONS
Variable value: -Xmx2G

Edit Environment Variable path:
Variable name: Path
Variable value: ;C:\Program Files\Java\jre6\bin;F:\JDK\bin;

I now see a successful OTP connection,

Sys.getenv("I_have_OTP")

[1] "TRUE"

I then retried the graph object and perhaps the new variables are good as it looks like it has gone past the initialisation, but there appears to be a Java version issue now. ? I am not familiar with Java so I was wondering if you could advise on what to try next?

Run the OTP and Build a graph object
log <- otp_build_graph(otp = path_otp, dir = path_data)

Error in otp_checks(otp = otp, dir = dir, router = router, graph = FALSE) :

In addition: Warning messages:
1: In system2("java", "-version", stdout = TRUE, stderr = TRUE) :
running command '"java" -version' had status 1
2: In otp_checks(otp = otp, dir = dir, router = router, graph = FALSE) :
NAs introduced by coercion
3: In otp_checks(otp = otp, dir = dir, router = router, graph = FALSE) :

Also see below the session information:

sessionInfo()
R version 3.5.1 (2018-07-02)
Platform: x86_64-w64-mingw32/x64 (64-bit)
Running under: Windows >= 8 x64 (build 9200)

Matrix products: default

locale:
[1] LC_COLLATE=English_New Zealand.1252 LC_CTYPE=English_New Zealand.1252
[3] LC_MONETARY=English_New Zealand.1252 LC_NUMERIC=C
[5] LC_TIME=English_New Zealand.1252

attached base packages:
[1] stats graphics grDevices utils datasets methods base

other attached packages:
[1] opentripplanner_0.1.0.0000

loaded via a namespace (and not attached):
[1] compiler_3.5.1 backports_1.1.4 clisymbols_1.2.0 tools_3.5.1 fs_1.2.6
[6] glue_1.3.1 rstudioapi_0.9.0 yaml_2.2.0 crayon_1.3.4 Rcpp_1.0.1
[11] usethis_1.5.0 checkmate_1.9.3 rlang_0.3.4

@kimnewzealand kimnewzealand referenced this issue 2 days ago
Open
Submission: opentripplanner #295
14 of 25 tasks complete
@mem48
Contributor
mem48 commented 2 days ago
Thanks for the detailed responce I'll try to reprodduce the issue, and get back to you asap

@mem48
Contributor
mem48 commented 2 days ago
A few first thoughts, the I_have_OTP is not necessary to use the package only for running the automated tests. The first thing that otp_build_graph does is run the internal function otp_checks. It might be useful to run this via opentripplanner:::otp_checks() to see if a more useful error message is produced. But it seems that you have several distinct problems.

On your first attempt Java tried to allocate 2097152KB of memory and failed (about 2 GB) which is the default. Perhaps you are running on a laptop with low memory? You could try memory = 1 in otp_build_graph. The demo data should build in 1 GB.

You make some changes to your path, I'm not quite sure what you did, do you have a link to the instructions you followed? But it now seems that R is unable to check the Java version which would suggest a problem with your java installation. You can check this manually in Comand Prompt with java -version

@kimnewzealand
Author
kimnewzealand commented 3 hours ago
thanks @mem48

I am trying on another laptop now so I will update thread soon

@kimnewzealand
Author
kimnewzealand commented 27 minutes ago
As an update, I have uninstalled and reinstalled Java version 8 on my machine and installed the package on another laptop with more memory.

I will ignore Sys.getenv("I_have_OTP") for now as you suggest.

I tried to run this command and received an error:
opentripplanner:::otp_checks()

Error in opentripplanner:::otp_checks() :
Assertion on 'dir' failed: No directory provided.

Despite the error with opentripplanner:::otp_checks(), I had success in building the graph by adding memory =1 on both laptops, see below message:
Run the OTP and Build a graph object
log <- otp_build_graph(otp = path_otp, dir = path_data, memory = 1)

Basic checks completed, building graph, this may take a few minutes
Graph built

I removed the path and variables as this didn't appear to be necessary on the other laptop. I suspect that the Java upgrade didn't work the first time round. Potentially Java needed a full uninstall first.
C:\Users\Home>java -version

java version "1.8.0_211"
Java(TM) SE Runtime Environment (build 1.8.0_211-b12)
Java HotSpot(TM) Client VM (build 25.211-b12, mixed mode, sharing)

Any further thoughts? Else this can probably be closed

[geanders]

Thanks @kimnewzealand!

[kimnewzealand]

I have raised another issue ( see here ITSLeeds/opentripplanner#30)

I have closed this issue now. See below for copy of the issue.
kimnewzealand commented 26 minutes ago •

I have successfully created a graph object but it seems like there is a potential issue with setting up the OTP instance. See below error message.

log <- otp_build_graph(otp = path_otp, dir = path_data, memory = 1)

Basic checks completed, building graph, this may take a few minutes
Graph built

otp_setup(otp = path_otp, dir = path_data)

2019-05-30 11:05:41 OTP is loading and may take a while to be useable
2019-05-30 11:11:05 OTP is taking an unusually long time to load, releasing R to your control

Trying the other alternative ports example
otp_setup(otp = path_otp, dir = path_data, port = 8801, securePort = 8802)

2019-05-30 11:11:06 OTP is loading and may take a while to be useable
2019-05-30 11:16:29 OTP is taking an unusually long time to load, releasing R to your control

@kimnewzealand kimnewzealand referenced this issue 23 minutes ago
Open
Submission: opentripplanner #295
14 of 25 tasks complete
kimnewzealand commented 27 seconds ago

O this one does not appear to be an issue. I tried the memory=1 trick again and it worked so I will close

otp_setup(otp = path_otp, dir = path_data,memory=1)

2019-05-30 12:02:35 OTP is loading and may take a while to be useable
Router http://localhost:8080/otp/routers/default exists
2019-05-30 12:05:47 OTP is ready to use Go to localhost:8080 in your browser to view the OTP

[mem48]

@kimnewzealand It seems that your problems are related to using a low power computer. It's good to test the package on a wide range of hardware. On the other hand, running a trip planner is an intrinsically difficult thing so you need a reasonably good computer. I've just updated the prerequisites vignette with a little section about hardware specs.

[mem48]

On the "OTP is taking an unusually long time to load, releasing R to your control" this might a bit confusing.

Because loading OTP takes a while, for my work I have large setups that can take several days to start up. I wrote the code so that R will wait for no more than 5 minutes before giving up. But that does not mean that OTP has stopped, in fact, OTP continues without R.

On reflection, this may be confusing for users, so perhaps I should change the behaviour to be the same as otp_build_graph, where R will wait until OTP is ready?

[kimnewzealand]

On the "OTP is taking an unusually long time to load, releasing R to your control" this might a bit confusing.

Because loading OTP takes a while, for my work I have large setups that can take several days to start up. I wrote the code so that R will wait for no more than 5 minutes before giving up. But that does not mean that OTP has stopped, in fact, OTP continues without R.

On reflection, this may be confusing for users, so perhaps I should change the behaviour to be the same as otp_build_graph, where R will wait until OTP is ready?

There is good news! I am at the point where I am set up and running and able to write up the review. I will do this mid to end of next week. I'll also have a think about these messages and add comments/suggestions in the review.

[kimnewzealand]

Thanks again for asking me to perform this review.

I have regular user perspective of someone who may be using this package personally or in a consultancy rather than from an academic or research perspective.

I spent the bulk of the time working though the Documentation, thinking of points where a new user may face issues using the package.

I ran the tests check but I did not use a larger data set as I also only have access to an 8GB RAM laptop. Therefore I did not test for other use cases or edge cases.

Let me know if you have any further questions.

Package Review

Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide

• As the reviewer I confirm that there are no conflicts of interest for me to review this work (If you are unsure whether you are in conflict, please speak to your editor before starting your review).

Documentation

The package includes all the following forms of documentation:

• A statement of need clearly stating problems the software is designed to solve and its target audience in README
• Installation instructions: for the development version of package and any non-standard dependencies in README
• Vignette(s) demonstrating major functionality that runs successfully locally
• Function Documentation: for all exported functions in R help
• Examples for all exported functions in R Help that run successfully locally
• Community guidelines including contribution guidelines in the README or CONTRIBUTING, and DESCRIPTION with URL, BugReports and Maintainer (which may be autogenerated via Authors@R).

For packages co-submitting to JOSS

• The package has an obvious research application according to JOSS's definition

The package contains a paper.md matching JOSS's requirements with:

• A short summary describing the high-level functionality of the software
• Authors: A list of authors with their affiliations
• A statement of need clearly stating problems the software is designed to solve and its target audience.
• References: with DOIs for all those that have one (e.g. papers, datasets, software).

Functionality

• Installation: Installation succeeds as documented.
• Functionality: Any functional claims of the software been confirmed.
• Performance: Any performance claims of the software been confirmed.

NOTE Only with 8MG Memory and given Isle of Wight sample dataset

• Automated tests: Unit tests cover essential functions of the package
  and a reasonable range of inputs and conditions. All tests pass on the local machine. ** see results below**
• Packaging guidelines: The package conforms to the rOpenSci packaging guidelines

Final approval (post-review)

• The author has responded to my review and made changes to my satisfaction. I recommend approving this package.

Estimated hours spent reviewing: 6-7 hours for prerquisite set up and 5 hours for review

• Should the author(s) deem it appropriate, I agree to be acknowledged as a package reviewer ("rev" role) in the package DESCRIPTION file.

---

Review Comments

General Comments

This is a great package providing an open source alternative for routing, to calculate routes using OTP from OSM, combine with other data and also create visualisations with this data - all within R.

As a user, once you get to Launch OTP and load the graph it is really exciting to see this part working visually with the OTP connection.

Package Review Checklist - Documentation

Here are some further notes on the Documentation checklist above.

Examples

I did not check off the Examples in the checklist above. I couldn't see a function nor find an example for it in the vignettes for this R script opentripplanner-package.R, is the R script needed?

The R help files are also comprehensive however the Examples, may need a review and update. They all include Not run comments which can probably be removed.

I appreciate that there is a sequential nature to most of the functions, perhaps use the vignette examples here or just provide a link to the getting started vignette?

Vignettes

Initially it took me half day (in total) to get setup due my laptop's memory issues with the Java and OTP tool prerequisites. However the prerequisites vignette has been updated with hardware considerations for the first time user based on issues raised.

The vignettes are fairly long and comprehensive which I think is needed for a first time user, given the initial set up required.

A comment regarding the target audience is that if the user is also new to R, then they may not be familiar with the different data formats and how to read those into R. From what I understand the opentripplanner package will read in any data format as long as it is downloaded into the expected folder structure.

Suggested updates

I have a couple of documentation suggestions:

• I suggest moving Getting Data for OTP section from the prerequisites vignette and the Isle of Wight data link to the opentripplanner: getting started vignette. It feels like this step follows the installation of the R package, but is related to Building an OTP Graph and the expectations around folder structure.
• Rename the the opentripplanner: getting started vignette to getting started
• Add the getting started to the Articles drop down menu
• Depending on rOpenSci's current view on Datacamp, perhaps remove the tutorial references
• Add a reference hyperlink for the .pbf format
• Add a reference hyperlink for the GeoTIFF format
• In this line Note that OTP only really needs the road network from the OSM. So for large areas, you may wish to remove unneeded data from the file using osmconvert and osmfilter and remove the and and explain where (OSM or R?) you can remove this unneeded data.
• Move this section from getting started to prerequisites as it feels like it part of the initial set up, and the user can work through this set up once before then installing or loading the R package.

Before you can use the package, you should set-up the OpenTripPlanner Java program. You will need to download the latest version of OTP from https://repo1.maven.org/maven2/org/opentripplanner/otp/ , at time of writing the latest version of OTP is version 1.3 so you would want to get https://repo1.maven.org/maven2/org/opentripplanner/otp/1.3.0/otp-1.3.0-shaded.jar

• Include a mention of the memory parameter or include another line of code in case the user encounters memory issues in this step:
  log <- otp_build_graph(otp = path_otp, dir = path_data, memory = 1)
• There are few places ion the README and vignettes where the . is missing at the end of the sentences

Community guidelines

I did not check off the Community guidelines in the checklist above. Although there is a Contributor code of conduct there is no markdown
guide for how to contribute.

Package Review Checklist - For packages co-submitting to JOSS

I did not check off the References in the checklist above as it is blank. Should this be populated with the Isle of Wight data source?

Suggested update:

• Update the wording in the link to SF data.frame to sf data frame as data.frame is the function whereas I believe the object is called a data frame.
• Update A full explanation is provided in the package vignettes link to https://itsleeds.github.io/opentripplanner/articles/

Package Review Checklist - Functionality

I checked all items in the Functionality checklist, however I have made some suggested updates below.

Suggested updates

I have a couple of functionality suggestions regarding messages:

• Ensure consistency across success messages, and include progress bars for example:

The success message is simple when running otp_build.

Basic checks completed, building graph, this may take a few minutes
Graph built

but the success message for otp_setup has a different format

otp_setup(otp = path_otp, dir = path_data, memory=1)
2019-06-05 14:47:22 OTP is loading and may take a while to be useable
Router http://localhost:8080/otp/routers/default exists
2019-06-05 14:50:07 OTP is ready to use Go to localhost:8080 in your browser to view the OTP

and the otp_plan has a nice progress bar but it is missing a message description of what's happening

|++++++++++++++++++++++++++++++++++++++++++++++++++| 100% elapsed = 14s

• The error message for ot_build_graph low memory is complicated, could this include a recommendation to add the memory parameter?

log <- otp_build_graph(otp = path_otp, dir = path_data )

Basic checks completed, building graph, this may take a few minutes
Failed to build graph with message:
Error occurred during initialization of VMCould not reserve enough space for 2097152KB object heap
Warning message:
In system(text, intern = TRUE) :
running command 'java -Xmx2G -jar "C:\Users\Home\AppData\Local\Temp\RtmpSYvN4O/OTP/otp.jar" --build "C:\Users\Home\AppData\Local\Temp\RtmpSYvN4O/OTP/graphs/default"' had status 1

• Perhaps don't include the timer cut-off of 5 minutes with this message, or include a parameter for this cut-off. The message is not useful as a user doesn't know what action to take:
  otp_setup(otp = path_otp, dir = path_data)

2019-05-30 11:05:41 OTP is loading and may take a while to be useable
2019-05-30 11:11:05 OTP is taking an unusually long time to load, releasing R to your control

Results of package checks and tests

-- R CMD check results ---------------------------------------------- opentripplanner 0.1.0.0000 ----
Duration: 6m 10.2s

checking for unstated dependencies in examples ... OK
WARNING
'qpdf' is needed for checks on size reduction of PDFs

checking top-level files ... NOTE
Non-standard file/directory found at top level:
'paper.bib'

checking R code for possible problems ... NOTE
Warning: : ... may be used in an incorrect context

otp_get_results: ... may be used in an incorrect context

0 errors √ | 1 warning x | 2 notes x

devtools::test()
Loading opentripplanner
Testing opentripplanner
√ | OK F W S | Context
| | 0 % ~calculating
| | 0 % ~calculating
| | 0 % ~calculating
| | 0 % ~calculating
√ | 29 3 | Test function without an OTP connection [21.2 s]

---

test_01_without_OTP.R:55: warning: otp_plan input validation
repeating fromPlace to match length of toPlace

test_01_without_OTP.R:61: warning: otp_plan input validation
repeating toPlace to match length of fromPlace

test_01_without_OTP.R:68: warning: otp_plan input validation
repeating toPlace to match length of fromPlace

√ | 0 | Test internal functions
√ | 2 | Test internal functions from otp-config.R
√ | 19 | Test internal functions from otp-plan.R [0.7 s]
√ | 10 3 | Download required files [56.4 s]

test_03_with_OTP.R:13: warning: download example data
'C:\Users\Home\AppData\Local\Temp\RtmpuSSdY2\otp' already exists

test_03_with_OTP.R:14: warning: download example data
'C:\Users\Home\AppData\Local\Temp\RtmpuSSdY2\otp\graphs' already exists

test_03_with_OTP.R:15: warning: download example data
'C:\Users\Home\AppData\Local\Temp\RtmpuSSdY2\otp\graphs\default' already exists

√ | 0 1 | Test the otp_build_graph function

test_03_with_OTP.R:60: skip: We can build an otp graph
Not running full test.

√ | 0 1 | Test the otp_setup function

test_03_with_OTP.R:71: skip: We can startup OTP
Not running full test.

√ | 0 3 | Test the otp_connect function [0.1 s]

test_03_with_OTP.R:81: skip: object returned when check is TRUE and router exists
Not running full test.

test_03_with_OTP.R:87: skip: correct message when check is TRUE and router exists
Not running full test.

test_03_with_OTP.R:93: skip: correct error when check is TRUE and router does not exist
Not running full test.

√ | 0 5 | Test the otp_plan function [0.1 s]

test_03_with_OTP.R:105: skip: basic routing
Not running full test.

test_03_with_OTP.R:116: skip: transit routing
Not running full test.

test_03_with_OTP.R:129: skip: no geometry routing
Not running full test.

test_03_with_OTP.R:140: skip: full elevation routing
Not running full test.

test_03_with_OTP.R:152: skip: batch routing
Not running full test.

√ | 0 2 | Test the otp_isochone function

test_03_with_OTP.R:166: skip: basic isochrone
Not running full test.

test_03_with_OTP.R:180: skip: nonsence isochrone
Not running full test.

√ | 0 5 | Test the otp_geocode function [0.1 s]

test_03_with_OTP.R:189: skip: basic geocode
Not running full test.

test_03_with_OTP.R:198: skip: geocode coords
Not running full test.

test_03_with_OTP.R:206: skip: geocode both
Not running full test.

test_03_with_OTP.R:214: skip: geocode nonsence
Not running full test.

test_03_with_OTP.R:222: skip: otp_stop
Not running full test.

|++++++++++++++++++++++++++++++++++++++++++++++++++| 100% elapsed = 01s
√ | 2 | Test the otp_plan function [1.1 s]
√ | 2 | Test the otp_isochone function [19.2 s]
√ | 2 | Test the otp_geocode function [0.5 s]

== Results =====================================================================
Duration: 101.6 s

OK: 66
Failed: 0
Warnings: 6
Skipped: 17

Woot!
Warning messages:
1: closing unused connection 4 (<-LAPTOP-222VVQ3R:11467)
2: closing unused connection 3 (<-LAPTOP-222VVQ3R:11467)

[geanders]

@Mem49: Since we lost one of the reviewers, I did the second review of this package, which I'm attaching here. I think that most of my comments and suggestions will be pretty quick to address (either small code changes or edits to documentation), so I'd recommend that you address those before submitting to CRAN, as I think you could do that pretty quickly.

Package Review

Please check off boxes as applicable, and elaborate in comments below. Your review is not limited to these topics, as described in the reviewer guide

• As the reviewer I confirm that there are no conflicts of interest for me to review this work (If you are unsure whether you are in conflict, please speak to your editor before starting your review).

Documentation

The package includes all the following forms of documentation:

• A statement of need clearly stating problems the software is designed to solve and its target audience in README
• Installation instructions: for the development version of package and any non-standard dependencies in README
• Vignette(s) demonstrating major functionality that runs successfully locally Note: Most code is set to not evaluate, in case the Java OTP set-up has not been done on the system, but evaluating all the code locally worked for me after doing the Java OTP set-up.
• Function Documentation: for all exported functions in R help
• Examples for all exported functions in R Help that run successfully locally
• Community guidelines including contribution guidelines in the README or CONTRIBUTING, and DESCRIPTION with URL, BugReports and Maintainer (which may be autogenerated via Authors@R).

For packages co-submitting to JOSS

• The package has an obvious research application according to JOSS's definition

The package contains a paper.md matching JOSS's requirements with:

• A short summary describing the high-level functionality of the software
• Authors: A list of authors with their affiliations
• A statement of need clearly stating problems the software is designed to solve and its target audience.
• References: with DOIs for all those that have one (e.g. papers, datasets, software).

Functionality

• Installation: Installation succeeds as documented.
• Functionality: Any functional claims of the software been confirmed.
• Performance: Any performance claims of the software been confirmed.
• Automated tests: Unit tests cover essential functions of the package
  and a reasonable range of inputs and conditions. All tests pass on the local machine.
  Note: I had some errors when I ran these locally (see below)
• Packaging guidelines: The package conforms to the rOpenSci packaging guidelines

Final approval (post-review)

• The author has responded to my review and made changes to my satisfaction. I recommend approving this package.

Estimated hours spent reviewing: 3 hours

• Should the author(s) deem it appropriate, I agree to be acknowledged as a package reviewer ("rev" role) in the package DESCRIPTION file.

---

Testing Error Messages

Here is the output from testing locally on my computer:

> devtools::check()
Updating opentripplanner documentation
Writing NAMESPACE
Loading opentripplanner
Writing NAMESPACE
── Building ─────────────────────────────────────────────────────────────────────────── opentripplanner ──
Setting env vars:
● CFLAGS    : -Wall -pedantic -fdiagnostics-color=always
● CXXFLAGS  : -Wall -pedantic -fdiagnostics-color=always
● CXX11FLAGS: -Wall -pedantic -fdiagnostics-color=always
──────────────────────────────────────────────────────────────────────────────────────────────────────────
✔  checking for file ‘/Users/georgianaanderson/Documents/ropensci_review/opentripplanner-master/DESCRIPTION’ ...
─  preparing ‘opentripplanner’:
✔  checking DESCRIPTION meta-information ...
─  installing the package to build vignettes
✔  creating vignettes (2.5s)
─  checking for LF line-endings in source and make files and shell scripts
─  checking for empty or unneeded directories
─  building ‘opentripplanner_0.1.0.0000.tar.gz’
   
── Checking ─────────────────────────────────────────────────────────────────────────── opentripplanner ──
Setting env vars:
● _R_CHECK_CRAN_INCOMING_USE_ASPELL_: TRUE
● _R_CHECK_CRAN_INCOMING_REMOTE_    : FALSE
● _R_CHECK_CRAN_INCOMING_           : FALSE
● _R_CHECK_FORCE_SUGGESTS_          : FALSE
── R CMD check ───────────────────────────────────────────────────────────────────────────────────────────
─  using log directory ‘/private/var/folders/83/ffl13dn505z0jvklj1y612kw0000gn/T/RtmpYlg7Lp/opentripplanner.Rcheck’
─  using R version 3.6.1 (2019-07-05)
─  using platform: x86_64-apple-darwin15.6.0 (64-bit)
─  using session charset: UTF-8
─  using options ‘--no-manual --as-cran’
✔  checking for file ‘opentripplanner/DESCRIPTION’
─  this is package ‘opentripplanner’ version ‘0.1.0.0000’
─  package encoding: UTF-8
✔  checking package namespace information ...
✔  checking package dependencies (869ms)
✔  checking if this is a source package
✔  checking if there is a namespace
✔  checking for executable files ...
✔  checking for hidden files and directories
✔  checking for portable file names ...
✔  checking for sufficient/correct file permissions
✔  checking serialization versions
✔  checking whether package ‘opentripplanner’ can be installed (1.3s)
✔  checking installed package size ...
✔  checking package directory ...
✔  checking for future file timestamps (1.4s)
✔  checking ‘build’ directory
✔  checking DESCRIPTION meta-information ...
N  checking top-level files ...
   Non-standard file/directory found at top level:
     ‘paper.bib’
✔  checking for left-over files ...
✔  checking index information ...
✔  checking package subdirectories ...
✔  checking R files for non-ASCII characters ...
✔  checking R files for syntax errors ...
✔  checking whether the package can be loaded ...
✔  checking whether the package can be loaded with stated dependencies ...
✔  checking whether the package can be unloaded cleanly ...
✔  checking whether the namespace can be loaded with stated dependencies ...
✔  checking whether the namespace can be unloaded cleanly ...
✔  checking dependencies in R code (508ms)
✔  checking S3 generic/method consistency (347ms)
✔  checking replacement functions ...
✔  checking foreign function calls ...
N  checking R code for possible problems (2.2s)
   Warning: <anonymous>: ... may be used in an incorrect context
   
   otp_get_results: ... may be used in an incorrect context
✔  checking Rd files ...
✔  checking Rd metadata ...
N  checking Rd line widths ...
   Rd file 'otp_dl_demo.Rd':
     \usage lines wider than 90 characters:
          url = "https://github.com/ITSLeeds/opentripplanner/releases/download/0.1/isle-of-wight-demo.zip")
   
   These lines will be truncated in the PDF manual.
✔  checking Rd cross-references ...
✔  checking for missing documentation entries ...
✔  checking for code/documentation mismatches ...
✔  checking Rd \usage sections (466ms)
✔  checking Rd contents ...
✔  checking for unstated dependencies in examples ...
✔  checking installed files from ‘inst/doc’ ...
✔  checking files in ‘vignettes’ ...
✔  checking examples (677ms)
✔  checking for unstated dependencies in ‘tests’ ...
─  checking tests ...
E  Running ‘testthat.R’ [2s/11s] (10.7s)
   Running the tests in ‘tests/testthat.R’ failed.
   Last 13 lines of output:
     ── 2. Failure: can get lsoa points (@test_03_with_OTP.R#14)  ─────────────────────────────────────────────
     nrow(lsoa) == 89 isn't true.
     
     ══ testthat results  ═════════════════════════════════════════════════════════════════════════════════════
     [ OK: 68 | SKIPPED: 17 | WARNINGS: 100 | FAILED: 2 ]
     1. Failure: test otp_checks without graph, with files (@test_02_internal_funcs.R#134) 
     2. Failure: can get lsoa points (@test_03_with_OTP.R#14) 
     
     Error: testthat unit tests failed
     In addition: Warning messages:
     1: In for (pkg in package) { :
       closing unused connection 5 (<-localhost:11066)
     2: In for (pkg in package) { :
       closing unused connection 4 (<-localhost:11066)
     Execution halted
✔  checking for unstated dependencies in vignettes ...
✔  checking package vignettes in ‘inst/doc’ ...
✔  checking re-building of vignette outputs (2.8s)
   
   See
     ‘/private/var/folders/83/ffl13dn505z0jvklj1y612kw0000gn/T/RtmpYlg7Lp/opentripplanner.Rcheck/00check.log’
   for details.
   
── R CMD check results ─────────────────────────────────────────────────── opentripplanner 0.1.0.0000 ────
Duration: 24.8s

❯ checking tests ...
  See below...

❯ checking top-level files ... NOTE
  Non-standard file/directory found at top level:
    ‘paper.bib’

❯ checking R code for possible problems ... NOTE
  Warning: <anonymous>: ... may be used in an incorrect context
  
  otp_get_results: ... may be used in an incorrect context

❯ checking Rd line widths ... NOTE
  Rd file 'otp_dl_demo.Rd':
    \usage lines wider than 90 characters:
         url = "https://github.com/ITSLeeds/opentripplanner/releases/download/0.1/isle-of-wight-demo.zip")
  
  These lines will be truncated in the PDF manual.

── Test failures ─────────────────────────────────────────────────────────────────────────── testthat ────

> library(testthat)
> library(opentripplanner)
> 
> test_check("opentripplanner")
trying URL 'https://github.com/ITSLeeds/opentripplanner/releases/download/0.1/isle-of-wight-demo.zip'
Content type 'application/octet-stream' length 8309996 bytes (7.9 MB)
==================================================
downloaded 7.9 MB

trying URL 'https://repo1.maven.org/maven2/org/opentripplanner/otp/1.4.0/otp-1.4.0-shaded.jar'
Content type 'application/java-archive' length 61838786 bytes (59.0 MB)
==================================================
downloaded 59.0 MB

── 1. Failure: test otp_checks without graph, with files (@test_02_internal_funcs.R#134)  ────────────────
otp_checks(...) isn't false.

── 2. Failure: can get lsoa points (@test_03_with_OTP.R#14)  ─────────────────────────────────────────────
nrow(lsoa) == 89 isn't true.

══ testthat results  ═════════════════════════════════════════════════════════════════════════════════════
[ OK: 68 | SKIPPED: 17 | WARNINGS: 100 | FAILED: 2 ]
1. Failure: test otp_checks without graph, with files (@test_02_internal_funcs.R#134) 
2. Failure: can get lsoa points (@test_03_with_OTP.R#14) 

Error: testthat unit tests failed
In addition: Warning messages:
1: In for (pkg in package) { :
  closing unused connection 5 (<-localhost:11066)
2: In for (pkg in package) { :
  closing unused connection 4 (<-localhost:11066)
Execution halted

1 error ✖ | 0 warnings ✔ | 3 notes ✖

It looks like the main concern is with getting the "lsoa" points file in (which I also
had problems with when running example code---see note below on that point). Also, any
idea on why 17 of the tests skipped? Is this a concern? Finally, the tests only ran
correctly if I started in a fresh R session. Maybe it would be useful to add some
code to the testing to clean-up any temporary directory and the environment before
running the tests to make the process a bit more robust (this isn't a major point, so
no worries if there's not an easy way to do this)?

Review Comments

Oh my goodness, what a very cool package this is! I really enjoyed playing around with it.

While it does require an initial investment from the user in terms of set-up, the updates to
the documentation put that in the range of many R users, so I think it has the potential to
reach a broad audience (and I don't see any obvious way to make the set-up process easier).

I identified some typos or areas in the documentation where different wording might be clearer.
I will submit a separate pull request for the package code to provide some suggestions for that.

I had the following specific comments, questions, and suggestions:

• For functions that save a file to the user's computer, it would be nice to add a line to
  documentation clarifying that something will be written to the user's computer, as well
  as perhaps a message printed when the function runs letting the user know a file is being
  written and where. I think the functions doing this are: otp_build_graph, otp_dl_demo,
  otp_dl_jar, and otp_write_config. With the nice documentation, I don't think this should
  be a shock to users that these functions are writing files to their computer, but I still
  think it's nice to make that very clear to users.
• For the help files, when router is an option (e.g., otp_build_graph), there is a note
  that it "must match with contents of dir". Could you clarify this point? I wasn't sure how
  to interpret it. Also, for functions with this option, it would be nice in the help file to
  include a link to more documentation on OTP and what routers are for OTP (and why you might
  want to use something other than the default).
• For the help file for otp_build_graph, what is an "analyst feature"? Again, perhaps a
  link to the OTP documentation on this point would be helpful in this helpfile.
• In the helpfile for otp_build_graph, it mentions in the Return Value section that this
  function returns and logs messages. Is it just printing out the returned messages, or does
  it write them in a file somewhere? The "log" term made me think it's writing these to a file
  somewhere, but then it wasn't clear to me where in the function code it was writing those
  messages (if it was). Perhaps change "log" to "print" here if the messages are just being
  printed out directly at the R console.
• I think it's wonderful that you have the example in the vignette set up so that everything's
  being downloaded to a temporary directory, so the user doesn't have a lot of clean-up to do
  from running the example. Also, you make it very clear that the user might want to use a
  non-temporary directory for a real project. I think it would be helpful to users if you could
  explain this point a bit more (perhaps with some example code somewhere of what you'd do for the
  set-up for a "real" project, where you want to be able to work on it more or reproduce the
  analysis later, rather than having everything delete when the R session is closed). For example,
  in one of the examples, you're referring to an OTP JAR file that's located in "C:/otp/otp.jar".
  Would this be a better place to save the OTP JAR when running otp_dl_jar if you think you'll
  want to use this program a lot? Otherwise, the user's re-downloading the JAR file each time they
  do an analysis, which doesn't seem necessary (have I interpreted what's going on here correctly?).
  This is a change that could wait for later versions (in terms of adding a bit to the vignette
  documentation), but I think at some point would be very helpful, as I imagine some users right
  now will end up using the example code with the temporary directory set-up every time they use
  this package, which would work but be a bit inefficient.
• For the otp_connect function, perhaps it would be helpful to paste the status code from
  check_router into the error message if that code is not 200? Maybe this extra information
  would help the user diagnose problems with connecting if they have them?
• For the otp_isochrone function, there is a note in the helpfile that this feature is known
  to not work correctly with any mode other than TRANSIT. If this is the case, perhaps it would
  be good to edit the assertion code in this function so that an error, or a least a message,
  is returned if the user tries to use any of the other modes? Also, this seems worth mentioning in
  the vignette section on Isochrones, as well as in the helpfile, as I imagine with the nice
  vignettes, many users will mostly learn about this package from the vignettes rather than the
  helpfiles. A warning only mentioned in the helpfile might not make it through to all users.
• For functions that use maxWalkDistance and minTransferTime, please add into the helpfiles
  the units that should be used for the numbers given for these parameters. Also, for values
  like these passed directly through to the Java OTP session, could you add links to the OTP
  help documentation so that a user could get a better idea how to use these options?
• For functions that use the mode parameter, can a vector of lengths longer than one
  be used to allow for multiple modes of transport? If not, could you clarify by change to
  something like "length-one character vector" rather than just "character vector" in the
  parameter definition in the helpfile? This might be another point where a link to the OTP
  documentation (directly in the parameter definition) would help users figure out how to use
  this parameter.
• There's a note in the helpfile for otp_plan about a bug related to the elevation
  profile only being returned for the first leg of the route. Is this still the case? From the
  vignette, it looks like you're able to get some elevation information throughout the route.
  Could you revisit the wording on this part of the helpfile and make sure it's still the case?
• In any of the examples that start with otp_connect, perhaps it would also make sense to
  include opt_stop() at the end of the example code? And in general, what are the repercussions
  if a user forgets to run opt_stop? I guess that the process will still be shut down the next
  time the person restarts their computer? It would be helpful to know if it is a very big deal
  if someone forgets to stop the Java process.
• What would happen if a user had two (or more) different versions of Java installed? Since
  OTP requires a very specific version of Java, this seems possible. In one of the checking functions,
  you have R running a system call to "java -version" and then checking the version number
  installed based on the output, so I'm curious what would happen with this piece of the code if
  the user has versions 1.8 and a different version installed.
• Really nice use of assertions throughout the code!
• The vignettes are just really, really great! Some of the parts that I found particularly
  nice include details about the memory and file sizes that users can expect for different parts of
  the process and connections with the OTP software's documentation. The examples in the
  "Advanced" vignette are also very nice and I think will attract many users to try out the package.
• For the otp_stop function, there's a typo in the message returned by the function.
• The naming convention for the functions (all start with otp_) is really thoughtful.
• Somewhere in the vignettes, it would be helpful to add a table with the name of all key
  package functions in one column and a brief description in the next.
• For the longer-term with this package, since it does require some set-up, it might be interesting
  to consider creating a container (e.g., with Docker) that someone could download with all the
  set-up incorporated (would that work?).
• It would be nice to address (briefly) reproducibility. It looks like the package defaults to
  always pull the same version of OTP, so perhaps mention that and that, as a result, if a user
  reruns code a while later, they should be using the same version of OTP?
• In the "Advanced Features" vignette, it might be helpful to clarify the language about
  error messages when you're referring to error messages from the Java OTP session that are
  transmitted by R. When I read "error messages", I assumed they were R error messages, so
  it was confusing why they wouldn't kill the process, but I'm pretty sure that these are
  instead error or warning messages from the Java process that pass through R and are printed out
  by R.
• What is an "OD matrix"? Origin-destination? Perhaps spell this out the first time you refer
  to it (unless it would be very clear to all transport researchers).
• For the "Advanced Features" vignette, you may want to consider showing what the first few lines
  of the created objects would look like, when you can. For example, it would be interesting to see
  what part of the routes_matrix looks like in the vignette.
• The "Known Issues" vignette is a very nice addition!
• For the examples in the help functions, I think it's reasonable that they're "Not run", as
  otherwise I think there will be some problems on CRAN, since it won't have the proper
  installation of the OTP JAR.
• Progress messages / bars are really nice
• For "fromPlace" and "toPlace", must the locations be given with a certain number of
  significant digits for the latitude and longitude? If so, please clarify in the help files.
• I got the following warning when I tried to read the centroids data directly from GitHub
  (but had no problem when I downloaded and read the file from a local location):

> lsoa <- sf::st_read("https://github.com/ITSLeeds/opentripplanner/releases/download/0.1/centroids.gpkg",
+                     stringsAsFactors = FALSE)
Reading layer `GeoJSONSeq' from data source `https://github.com/ITSLeeds/opentripplanner/releases/download/0.1/centroids.gpkg' using driver `GeoJSONSeq'
Simple feature collection with 0 features and 0 fields
bbox:           xmin: NA ymin: NA xmax: NA ymax: NA
epsg (SRID):    4326
proj4string:    +proj=longlat +datum=WGS84 +no_defs
There were 50 or more warnings (use warnings() to see the first 50)

• Once of the vignettes uses spread from tidyr. This function is now deprecated---could you
  update to use pivot_wider?

[mem48]

Thanks @geanders I'll get on it and let you know when I've made the necessary fixes.

[mem48]

@geanders lots to go through, but I think I've fixed most of these.

It looks like the main concern is with getting the "lsoa" points file in (which I also had problems with when running example code---see note below on that point). Also, any idea on why 17 of the tests skipped? Is this a concern? Finally, the tests only ran correctly if I started in a fresh R session

I fixed some badly designed tests, the skips are because you don't have Sys.getenv("I_have_OTP") see https://github.com/ITSLeeds/opentripplanner/blob/master/README.md Its a simple toggle to disable tests that require the correct version of Java.

For functions that save a file to the user's computer, it would be nice to add a line to
documentation clarifying that something will be written to the user's computer.

Warning message added giving file path

For the help files, when router is an option (e.g., otp_build_graph), there is a note
that it "must match with contents of dir". Could you clarify this point? I wasn't sure how
to interpret it. Also, for functions with this option, it would be nice in the help file to
include a link to more documentation on OTP and what routers are for OTP (and why you might
want to use something other than the default).

Updated the vignette with explanation.

For the help file for otp_build_graph, what is an "analyst feature"? Again, perhaps a
link to the OTP documentation on this point would be helpful in this helpfile.

Updated the vignette with explanation.

In the helpfile for otp_build_graph, it mentions in the Return Value section that this
function returns and logs messages. Is it just printing out the returned messages, or does
it write them in a file somewhere? The "log" term made me think it's writing these to a file
somewhere, but then it wasn't clear to me where in the function code it was writing those
messages (if it was). Perhaps change "log" to "print" here if the messages are just being
printed out directly at the R console.

Updated the help with explanation. There is no log file, just printed to console.

I think it's wonderful that you have the example in the vignette set up so that everything's
being downloaded to a temporary directory, so the user doesn't have a lot of clean-up to do
from running the example. Also, you make it very clear that the user might want to use a
non-temporary directory for a real project. I think it would be helpful to users if you could
explain this point a bit more (perhaps with some example code somewhere of what you'd do for   the
set-up for a "real" project, where you want to be able to work on it more or reproduce the
analysis later, rather than having everything delete when the R session is closed). For example,
in one of the examples, you're referring to an OTP JAR file that's located in "C:/otp/otp.jar".
Would this be a better place to save the OTP JAR when running otp_dl_jar if you think you'll
want to use this program a lot? Otherwise, the user's re-downloading the JAR file each time they
do an analysis, which doesn't seem necessary (have I interpreted what's going on here correctly?).

Yes you are right, Updated the vignette with explination.

For the otp_connect function, perhaps it would be helpful to paste the status code from
check_router into the error message if that code is not 200?

Done

For the otp_isochrone function, there is a note in the helpfile that this feature is known to not work correctly with any mode other than TRANSIT.

It is, but this is something that will be fixed by the main OTP team. I think giving extra warnings based on the version of OTP is a bit beyond the remit of the R package. The R package is doing its job correctly and OTP provides their own documentation which overs how OTP itself works.

For functions that use maxWalkDistance and minTransferTime, please add into the helpfiles the units that should be used for the numbers given for these parameters. 

Done

Also, for values like these passed directly through to the Java OTP session, could you add links to the OTP help documentation so that a user could get a better idea how to use these options?

They are rewriting their documentation at the moment, so perhaps best left until later?

For functions that use the mode parameter, can a vector of lengths longer than one be used to allow for multiple modes of transport?

Help updated

There's a note in the helpfile for otp_plan about a bug related to the elevation profile only being returned for the first leg of the route. Is this still the case?

It is, the vignette only shows a single leg journey, the bug occurs when you get a walk, transit, walk route. You only get elevation data for the first walk leg.

In any of the examples that start with otp_connect, perhaps it would also make sense to include opt_stop() at the end of the example code? 

It is not necessary to stop OPT, in most cases, it stops automatically when RStudio is closed. The function is mostly there for testing, allowing users to force OTP to stop if needed.

What would happen if a user had two (or more) different versions of Java installed?

With multiple versions of java there is still a "default" one. I haven't tested this extensively as users who can install multiple version of java are probably expert users.

Somewhere in the vignettes, it would be helpful to add a table with the name of all key

package functions in one column and a brief description in the next.

Do you mean https://itsleeds.github.io/opentripplanner/reference/index.html ?

For the longer-term with this package, since it does require some set-up, it might be interesting to consider creating a container (e.g., with Docker)

Good idea, but then there would need to be a Docker tutorial. I think about it for the long term.

It would be nice to address (briefly) reproducibility. It looks like the package defaults to always pull the same version of OTP, so perhaps mention that and that, as a result, if a user reruns code a while later, they should be using the same version of OTP?

otp_dl_jar() has a url argument that selects the version of OTP. I had assumed that once new versions come out I would update the url once I've checked that the package is working well with the new version.

In the "Advanced Features" vignette, it might be helpful to clarify the language about error messages when you're referring to error messages from the Java OTP session that are transmitted by R. 

Updated vignette

What is an "OD matrix"?

Updated vignette

For the "Advanced Features" vignette, you may want to consider showing what the first few lines

of the created objects would look like, when you can.

To do that I will have to actually set up a working instance of OTP when building the vignettes, which is hard to make work on the servers which don't usually have Java. I could put in some screenshots if you think it is important.

For the examples in the help functions, I think it's reasonable that they're "Not run", as otherwise, I think there will be some problems on CRAN, since it won't have the proper installation of the OTP JAR.

All the example should have the \dontrun tag, can you give a specific example?

For "fromPlace" and "toPlace", must the locations be given with a certain number of significant digits for the latitude and longitude?

It doesn't matter, lest digits is just less precise.

I got the following warning when I tried to read the centroids data directly from GitHub (but had no problem when I downloaded and read the file from a local location):

I can't reproduce, could your firewall be blocking R from downloading files?

[geanders]

@mem48 : Overall, I think these responses have satisfied most of my comments and suggestions. The only small follow-ups are (in some cases, these are just clarifications of my original comments or noting where I think my original comment has been satisfactorily addressed):

I fixed some badly designed tests, the skips are because you don't have Sys.getenv("I_have_OTP") see https://github.com/ITSLeeds/opentripplanner/blob/master/README.md Its a simple toggle to disable tests that require the correct version of Java.

Thanks! I thought I had set this, but I double checked and I think I had a typo in the name I used for the environmental variable name. I will re-check the tests on my computer.

All your updates to helpfiles and vignettes in response to my suggestions on those are great.

It is, but this is something that will be fixed by the main OTP team. I think giving extra warnings based on the version of OTP is a bit beyond the remit of the R package. The R package is doing its job correctly and OTP provides their own documentation which overs how OTP itself works.

Fine, this sounds very reasonable. Could you add the caveat that you have in the helpfile on this to the vignette, as well, though? Since the package's vignettes are so good and comprehensive, I think many users will rely on those and might not read the helpfiles much. This seems like an important caveat for a user to know, even if it is the result right now of a bug in the main OTP software. Right now, unless the user overrides a default in opentripplanner, they will always be downloading the version of OTP with this bug, right? That being the case, I think this package is tied closely enough to a specific version that, if there is a bug (or limitation) in that version, it is worth making sure the user is aware.

They are rewriting their documentation at the moment, so perhaps best left until later?

That sounds reasonable to me.

It is, the vignette only shows a single leg journey, the bug occurs when you get a walk, transit, walk route. You only get elevation data for the first walk leg.

I see. That sounds fine, then.

It is not necessary to stop OPT, in most cases, it stops automatically when RStudio is closed. The function is mostly there for testing, allowing users to force OTP to stop if needed.

That sounds fine. I agree with not needing to make any changes based on my original comment on this, then.

With multiple versions of java there is still a "default" one. I haven't tested this extensively as users who can install multiple version of java are probably expert users.

This sounds reasonable.

Do you mean https://itsleeds.github.io/opentripplanner/reference/index.html ?

Yes! Sorry I missed this. Great that it's there.

Good idea, but then there would need to be a Docker tutorial. I think about it for the long term.

Sounds reasonable.

otp_dl_jar() has a url argument that selects the version of OTP. I had assumed that once new versions come out I would update the url once I've checked that the package is working well with the new version.

Sounds reasonable.

To do that I will have to actually set up a working instance of OTP when building the vignettes, which is hard to make work on the servers which don't usually have Java. I could put in some screenshots if you think it is important.

This is fine to leave as-is. It would be nice, but not worth this amount of trouble.

All the example should have the \dontrun tag, can you give a specific example?

Sorry, I don't think I was clear here. I was trying to say that I think it's fine to have the helpfile examples not run, since the system needs the right set-up with Java to run. I was making this comment because there had been an earlier concern in the reviews on this point, and I just wanted to say that I don't think that's a big problem and I think the current version is reasonable. Sorry I wasn't clearer on that comment.

I can't reproduce, could your firewall be blocking R from downloading files?

I will follow up on this and see if it looks like something with my firewall.

[mem48]

I've added the comment to the isochrones vignette. I think that is everything covered.
I will go over and look for any typos etc. Thanks for all your help.

[mem48]

@geanders I've finished spell checking the documentation. I think the package is good to go unless you want any other changes?

[geanders]

@mem48 : Yes, as a reviewer, I am very happy with the response to all my questions. I think this is a really great package, with especially thoughtful vignettes.

Putting my editor hat back on... @kimnewzealand : from your comments, it looks like you are satisfied with the responses and changes for your comments---could you confirm that's right and, if so, check the box on "The author has responded to my review and made changes to my satisfaction. I recommend approving this package" in you original review comment in this thread?

[kimnewzealand]

Having a look now @mem48 @geanders ....

Firstly I have now checked off the References now based on the answer above and your review

[kimnewzealand]

There are few suggestions to follow up on, please let me know if you had a chance to review these @mem48:

Depending on rOpenSci's current view on Datacamp, perhaps remove the tutorial references
Add a reference hyperlink for the .pbf format
Add a reference hyperlink for the GeoTIFF format
In this line Note that OTP only really needs the road network from the OSM. So for large areas, you may wish to remove unneeded data from the file using osmconvert and osmfilter and remove the and and explain where (OSM or R?) you can remove this unneeded data.
Move this section from getting started to prerequisites as it feels like it part of the initial set up, and the user can work through this set up once before then installing or loading the R package.
Before you can use the package, you should set-up the OpenTripPlanner Java program. You will need to download the latest version of OTP from https://repo1.maven.org/maven2/org/opentripplanner/otp/ , at time of writing the latest version of OTP is version 1.3 so you would want to get https://repo1.maven.org/maven2/org/opentripplanner/otp/1.3.0/otp-1.3.0-shaded.jar

Include a mention of the memory parameter or include another line of code in case the user encounters memory issues in this step:
log <- otp_build_graph(otp = path_otp, dir = path_data, memory = 1)
There are few places ion the README and vignettes where the . is missing at the end of the sentences

[kimnewzealand]

I have also updated the checkmark for the Community guidelines given:
Bug reports and comments are welcome as Github Issues and code submissions as Pull Requests.

[mem48]

Hi @kimnewzealand

Add a reference hyperlink for the .pbf format
Add a reference hyperlink for the GeoTIFF format
In this line Note that OTP only really needs the road network from the OSM. .....
There are few places ion the README and vignettes where the . is missing at the end of the sentences

These are done.

The getting data and the bits about downloading the .jar file have been rewritten too. In particular I've added a new functions otp_dl_jar() and otp_dl_demo() which simplify these stages.

I would prefer to keep the section on "getting your own data" in the getting started vignette, because I think most users would want to go through the vignettes with the demo data first then come back to using their own data.

If finding your own data is made part of the prerequisites, then new users may think that they need their own data to complete the rest of the tutorials.

Include a mention of the memory parameter or include another line of code in case the user encounters memory issues in this step:
log <- otp_build_graph(otp = path_otp, dir = path_data, memory = 1)

Memory use is now explained in the hardware section of the prerequisites vignette and repeated in the getting started vignette.

I've read the readme and added punctuation as required.

[kimnewzealand]

Thanks for the response.
I've checked the review as done @geanders @mem48

[geanders]

Approved! Thanks @mem48 for submitting and @kimnewzealand for your review!

To-dos:

• Transfer the repo to rOpenSci's "ropensci" GitHub organization under "Settings" in your repo. I have invited you to a team that should allow you to do so. You'll be made admin once you do.
• Add the rOpenSci footer to the bottom of your README
  " [![ropensci_footer](https://ropensci.org/public_images/ropensci_footer.png)](https://ropensci.org)"
• Fix all links to the GitHub repo to point to the repo under the ropensci organization.
• If you already had a pkgdown website, fix its URL to point to https://docs.ropensci.org/package_name and deactivate the automatic deployment you might have set up, since it will not be built centrally like for all rOpenSci packages, see http://devdevguide.netlify.com/#docsropensci. In addition, in your DESCRIPTION file, include the docs link in the URL field alongside the link to the GitHub repository, e.g.: URL: https://docs.ropensci.org/foobar (website) https://github.com/ropensci/foobar
• Add a mention of the review in DESCRIPTION via rodev::add_ro_desc().
• Fix any links in badges for CI and coverage to point to the ropensci URL. We no longer transfer Appveyor projects to ropensci Appveyor account so after transfer of your repo to rOpenSci's "ropensci" GitHub organization the badge should be [![AppVeyor Build Status](https://ci.appveyor.com/api/projects/status/github/ropensci/pkgname?branch=master&svg=true)](https://ci.appveyor.com/project/individualaccount/pkgname).
• We're starting to roll out software metadata files to all ropensci packages via the Codemeta initiative, see https://github.com/ropensci/codemetar/#codemetar for how to include it in your package, after installing the package - should be easy as running codemetar::write_codemeta() in the root of your package.
• Activate Zenodo watching the repo
• Tag and create a release so as to create a Zenodo version and DOI
• Submit to JOSS at https://joss.theoj.org/papers/new, using the rOpenSci GitHub repo URL. When a JOSS "PRE REVIEW" issue is generated for your paper, add the comment: This package has been reviewed by rOpenSci: https://LINK.TO/THE/REVIEW/ISSUE, @ropensci/editors

Should you want to acknowledge your reviewers in your package DESCRIPTION, you can do so by making them "rev"-type contributors in the Authors@R field (with their consent). More info on this here.

Welcome aboard! We'd love to host a blog post about your package - either a short introduction to it with one example or a longer post with some narrative about its development or something you learned, and an example of its use. If you are interested, review the instructions, and tag @stefaniebutland in your reply. She will get in touch about timing and can answer any questions.

We've put together an online book with our best practice and tips, this chapter starts the 3d section that's about guidance for after onboarding. Please tell us what could be improved, the corresponding repo is here.

[mem48]

Hi @geanders I've accepted your invitation and moved the package to ropensci but I seem to have lost control of the package. I can start making the changes to links etc locally but I'm unable to commit them to github. Did I do something wrong?

[geanders]

@mem48 : No, you didn't do anything wrong! You should now have admin access to the package again now. Please let me know if that's not the case.

[mem48]

Hi @geanders, I think I've done all the tasks on your list.

[mem48]

Hi @stefaniebutland I'd be happy to write a short introduction to the package. Is it ok to rework some of the vignettes or does it have to completly original content?

[stefaniebutland]

Hi @mem48. It does have to be original content. For something short, you could consider writing a tech note, "Package updates, technical know-how, and small announcements related to rOpenSci software development".

fyi @Robinlovelace is writing a big-picture post about stplanr for publication in December. Noting in case that influences your content.

[Robinlovelace]

Yes this should go in there, all these things join up and a big picture overview is just what's needed right now. More soon, xmas deadline noted. By the way huge congratulations Malcolm!

[maelle]

Closing this, which doesn't prevent the convo about blog posts from continuing. 😉

[Robinlovelace]

Great to see this, thanks everyone (and sorry @stefaniebutland for being slack on slack and with writing, have been prioritising academic projects and holiday and unfortunately the blog post has suffered). One question where I imagine rOpenSci people can help is release on CRAN. I believe doors open again today for submissions. Any pointers @mem48 on barriers to this being up?

Looking forward to https://cran.r-project.org/package=opentripplanner being live!

[stefaniebutland]

have been prioritising academic projects and holiday

wisdom!

One question where I imagine rOpenSci people can help is release on CRAN.

rOpenSci Slack is a great place to ask questions like this. Lots of CRAN experience and specific helpful answers

When you're ready, email me for blog post follow up

Cheers

[mem48]

@stefaniebutland I've written a short blog post but I can't find your email address. Can you tell me where to find it or email me at m.morgan1@leeds.ac.uk

[stefaniebutland]

Excellent! Here are the instructions for submitting a post: https://github.com/ropensci/roweb2#contributing-a-blog-post

Once submitted, I will review and set a publication date. Please set date as 2020-02-04 for now.

Happy to answer any questions.