Add documentation for PlannerResource#mode by leonardehrenfried · Pull Request #2809 · opentripplanner/OpenTripPlanner

leonardehrenfried · 2019-08-16T11:50:13Z

Since this is purely about documentation I took the liberty to remove the PR checklist. I hope this is acceptable.

I was confused about the mode parameter, and especially about the qualifiers like BICYCLE_PARK and BICYCLE_RENT so I experimented and dug through the code.

The documentation contained in the PR are the result of my findings.

I'm very happy to discuss the format and content.

Thanks for OTP - it's a great piece of software!

evansiroky · 2019-08-20T01:03:58Z

+     * <ul>
+     *  <li>WALK</li>
+     *  <li>TRANSIT: General catch-all for all public transport modes.</li>
+     *  <li>BICYCLE: Taking a bicycle onto the public transport and cycling from the arrival station to the destination.</li>


Transit isn't required with the BICYCLE mode. It's possible to send just BICYCLE and get a bike-only result.

evansiroky

The enunciate API docs certainly leaves a lot to be desired. I think it's a good idea to improve this piecemeal and I like the focus of the PR thus far. In its current form, I think this raises more questions than answers and could use the following improvements:

An explanation of each of the modes and their prerequisites.
An overall note that the availability of these modes depends on each OTP deployment and that some modes may not be available in certain deployments.
An explanation of what mode qualifiers are and how although most won't make sense (eg FERRY_PARK) there are certain known combinations (eg BICYCLE_RENT, BICYCLE_PARK, CAR_PARK).

leonardehrenfried · 2019-08-20T08:29:51Z

I appreciate the concern about completeness.

Do you think it makes sense to put all of this information in the enunciate docs or would it be better to just give a list of possible values and link to a more fully-fledged documentation page (which I'd volunteer to write)?
On the other hand, if we start putting more information in that place, it might serve as an encouragement for other parameters to be documented more extensively.

An explanation of what mode qualifiers are and how although most won't make sense (eg FERRY_PARK) there are certain known combinations (eg BICYCLE_RENT, BICYCLE_PARK, CAR_PARK).

There are also the qualifiers KEEP and HAVE, but they don't seem to be used. Am I seeing this correctly?

evansiroky · 2019-08-20T18:24:36Z

I like the idea of only adding in a list of known mode strings and then putting in a link to more detailed documentation. I don't know what a good place for that would be though. Maybe a new section about modes in the Configuration docs (http://docs.opentripplanner.org/en/latest/Configuration/ which is derived from docs/Configuration.md)?

Regarding the KEEP and HAVE qualifiers, I also can confirm they aren't used anywhere and I think it'd be a good idea to remove them to avoid any confusion. However, I'll ping the original author of those qualifiers (@abyrd) just in case.

leonardehrenfried · 2019-08-21T09:00:58Z

I've move the more extensive discussion to Configuration.md. Can you check it for correctness?

evansiroky · 2019-08-21T16:48:45Z

+
+- `WALK`: Walking some or all of the route.
+
+    _Prerequisite_: The maximum walking distance configured has not been exceeded.


This prerequisite is not always true and is rather complex to explain why, so I'd just remove it altogether.

evansiroky · 2019-08-21T16:51:48Z

+
+    _Prerequisite:_ Bicycle parking stations near the station and visible to OTP by enabling the property `staticBikeParkAndRide` during graph build.
+
+- `CAR`: Driving your own car the entirety of the route.


If the modes CAR,TRANSIT are sent over, this actually implies a "Kiss and Ride" type of search where a car will be driven through the street network from the origin until an opportune time when a transition to walking and then taking transit will occur. It might be good to note that.

evansiroky · 2019-08-21T16:53:22Z

+
+- `CAR_PARK`: Driving a car to the park-and-ride facilities near a station and taking public transport.
+
+    _Prerequisite:_ Park-and-ride areas near the station and visible to OTP by enabling the property `staticParkAndRide` during graph build.


The staticParkAndRide is set to true by default. It might also be worth noting that there need to be Park and Rides defined in the OpenStreetMap data that is being used.

Also, it should be noted that for Park & Ride searches at least one transit mode should also be included, otherwise this will behave as if only the CAR mode were sent over.

evansiroky · 2019-08-21T17:08:11Z

+
+- `FERRY`: Taking a ferry.
+
+- `GONDOLA`: Taking a hand-powered rowing boat.


The 7 above types, in general, refer to the route_type as defined in the GTFS specification. As such, the definition of BUS isn't quite right, the GONDOLA is not correct and FUNICULAR is missing.

evansiroky · 2019-08-21T17:10:41Z

+
+- `BICYCLE_PARK`: Leaving the bicycle at the departure station and walking from the arrival station to the destination. 
+
+    _Prerequisite:_ Bicycle parking stations near the station and visible to OTP by enabling the property `staticBikeParkAndRide` during graph build.


It should be noted that for Bike Park & Ride searches at least one transit mode should also be included, otherwise this will behave as if only the BICYCLE mode were sent over.

leonardehrenfried · 2019-08-22T11:33:24Z

I've incorporated the review feedback.

Could you shed some light on how the mode AIRPLANE works?

evansiroky · 2019-08-22T17:59:01Z

+
+- `BICYCLE_PARK`: Leaving the bicycle at the departure station and walking from the arrival station to the destination.
+
+    This mode needs to be combined with at least one transit mode (or `TRANSIT`) and with `WALK` otherwise it behaves like an ordinary bicycle journey.


The WALK mode is actually not required in order to make requests with the BICYCLE_PARK mode perform walking from the bike parking areas to transit and also walking after transit.

evansiroky · 2019-08-22T18:03:07Z

+
+- `CAR`: Driving your own car the entirety of the route. 
+
+    If this is combined with `TRANSIT` it can return routes with a 


I'd change this to be more explicit and say that If this is combined with TRANSIT or some other combination of transit modes the response will return routes...

evansiroky · 2019-08-22T18:10:46Z

I've incorporated the review feedback.

Could you shed some light on how the mode AIRPLANE works?

This PR is looking better and better.

The AIRPLANE mode behaves just like any other transit mode (those modes are TRAM, SUBWAY, RAIL, BUS, FERRY, CABLE_CAR, GONDOLA, FUNICULAR). Although I've never ran an OTP instance using that mode, I believe airplane routes are defined in the GTFS like any other transit route except that the route type uses route_type=1100 as noted in the Extended GTFS Route Types document.

leonardehrenfried · 2019-08-22T19:56:14Z

Thanks for the explanation.

I've amended the PR once more.

evansiroky

Awesome, thanks for all the amendments. This adds a lot of useful and clear information.

leonardehrenfried · 2019-08-27T09:57:08Z

@evansiroky I found another piece of documentation which was showing the wrong default: areaVisbility. I also added a more extensive description of what areaVisibility means.

Could you please take a look?

abyrd · 2019-08-28T11:10:44Z

Thanks @leonardehrenfried for the work on updating documentation. There are indeed many things in OTP that are not documented so it's good to make incremental improvements. This whole "mode qualifier" system was never totally completed - it was originally intended to allow specifying chains of allowed modes e.g. something like (WALK or BIKE) -> (TRANSIT) -> (BIKE or CAR). That depended on our "path parser" system which was eventually removed to reduce complexity.

Then on top of that certain mode combinations were "recycled" to trigger things like park-and-ride or kiss-and-ride. So the end result is quite ad-hoc and I'm a bit hesitant to present it as if it were a coherent system, but of course at some point we need to describe things as they are.

leonardehrenfried · 2019-09-05T14:16:50Z

@abyrd Would you like me to change the documentation or is there anything else I can do to move this PR along?

Would you like me to add your explanation?

abyrd · 2019-09-11T07:09:20Z

 `streets` | Include street input files (OSM/PBF) | boolean | true | 
 `embedRouterConfig` | Embed the Router config in the graph, which allows it to be sent to a server fully configured over the wire | boolean | true |
-`areaVisibility` | Perform visibility calculations on OSM areas (these calculations can be time consuming) | boolean | true |
+`areaVisibility` | Perform visibility calculations. If this is `true` OTP attempts to calculate a path straight through an OSM area using the shortest way rather than around the edge of it. (These calculations can be time consuming). | boolean | false |


The computations should not be time consuming during routing, only during graph build. We should clarify that here.

abyrd · 2019-09-11T07:13:28Z

+     * <p>The set of modes that a user is willing to use, with qualifiers stating whether vehicles should be parked, rented, etc.</p>
+     * <p>The possible values of the comma-separated list are:</p>
+     *
+     * <ul>


I'm a little hesitant to create such a list in documentation that we then have to manually maintain. Considering the limited number of "Qualified Modes" actually in use, it seems like we could just define an enum for all these values. But that would be a bigger refactor, I guess this is the best incremental improvement.

I spent days debugging an issue with an unhappy customer that traced back to a lack of documentation of what value values were for leg.mode. Good, complete docs matter.

t2gran · 2019-09-13T08:09:23Z

@abyrd Should this be cherry picked into OTP2 ?

abyrd · 2019-09-13T09:53:18Z

@t2gran yes, we should try to keep the documentation in sync. I expect that eventually when we do a 2.0 release we'll do a final sync of the documentation, Readme etc. to make sure all relevant changes are carried over. But like with the code changes, it will make any later merging easier if we cherry pick changes over right away.

abyrd · 2019-09-13T09:54:46Z

@t2gran also feel free to cherry pick documentation-only changes like this straight into dev-2.x, or merge equivalent PRs into 2.x without review as long as the equivalent 1.x PR has been approved and merged.

t2gran · 2019-09-13T10:39:02Z

Thank you I will cherry pick this one then.

leonardehrenfried requested a review from a team August 16, 2019 11:50

evansiroky reviewed Aug 20, 2019

View reviewed changes

Comment thread src/main/java/org/opentripplanner/api/common/RoutingResource.java

evansiroky suggested changes Aug 20, 2019

View reviewed changes

leonardehrenfried force-pushed the dev-1.x branch from 539a99c to 553c16a Compare August 21, 2019 08:49

leonardehrenfried force-pushed the dev-1.x branch from 553c16a to cb28af9 Compare August 21, 2019 13:12

evansiroky reviewed Aug 21, 2019

View reviewed changes

leonardehrenfried force-pushed the dev-1.x branch 2 times, most recently from af844a3 to 4ea61cf Compare August 22, 2019 11:30

leonardehrenfried force-pushed the dev-1.x branch from 4ea61cf to 0a82a9f Compare August 22, 2019 11:45

evansiroky reviewed Aug 22, 2019

View reviewed changes

leonardehrenfried force-pushed the dev-1.x branch from 0a82a9f to 82a48f2 Compare August 22, 2019 19:55

evansiroky previously approved these changes Aug 22, 2019

View reviewed changes

leonardehrenfried dismissed evansiroky’s stale review via f0c0e8c August 24, 2019 19:27

evansiroky previously approved these changes Aug 29, 2019

View reviewed changes

Add documentation for PlannerResource#mode

bd0622e

leonardehrenfried added 3 commits September 5, 2019 16:18

Move complete discussion of mode parameter into Documentation.md

86f9644

Add changelog

a478add

Improve documentation for areaVisibility

5f067a0

leonardehrenfried dismissed evansiroky’s stale review via 5f067a0 September 5, 2019 14:19

leonardehrenfried force-pushed the dev-1.x branch from 3c83f76 to 5f067a0 Compare September 5, 2019 14:19

evansiroky approved these changes Sep 10, 2019

View reviewed changes

abyrd approved these changes Sep 11, 2019

View reviewed changes

abyrd merged commit ead9970 into opentripplanner:dev-1.x Sep 11, 2019

t2gran added the X OTP1 ~ Not in use any more ~ Fix or backport to the 1.x version of OTP label Sep 13, 2019

t2gran mentioned this pull request Sep 13, 2019

Add documentation for PlannerResource#mode (cherry picked) #2830

Merged

evansiroky mentioned this pull request Oct 8, 2019

API docs for new modes ibi-group/OpenTripPlanner#17

Merged


		- `WALK`: Walking some or all of the route.

		_Prerequisite_: The maximum walking distance configured has not been exceeded.


		_Prerequisite:_ Bicycle parking stations near the station and visible to OTP by enabling the property `staticBikeParkAndRide` during graph build.

		- `CAR`: Driving your own car the entirety of the route.


		- `CAR_PARK`: Driving a car to the park-and-ride facilities near a station and taking public transport.

		_Prerequisite:_ Park-and-ride areas near the station and visible to OTP by enabling the property `staticParkAndRide` during graph build.


		- `FERRY`: Taking a ferry.

		- `GONDOLA`: Taking a hand-powered rowing boat.


		- `BICYCLE_PARK`: Leaving the bicycle at the departure station and walking from the arrival station to the destination.

		_Prerequisite:_ Bicycle parking stations near the station and visible to OTP by enabling the property `staticBikeParkAndRide` during graph build.


		- `BICYCLE_PARK`: Leaving the bicycle at the departure station and walking from the arrival station to the destination.

		This mode needs to be combined with at least one transit mode (or `TRANSIT`) and with `WALK` otherwise it behaves like an ordinary bicycle journey.


		- `CAR`: Driving your own car the entirety of the route.

		If this is combined with `TRANSIT` it can return routes with a

Uh oh!

Conversation

leonardehrenfried commented Aug 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

evansiroky left a comment

Choose a reason for hiding this comment

Uh oh!

leonardehrenfried commented Aug 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

evansiroky commented Aug 20, 2019

Uh oh!

leonardehrenfried commented Aug 21, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leonardehrenfried commented Aug 22, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evansiroky commented Aug 22, 2019

Uh oh!

leonardehrenfried commented Aug 22, 2019

Uh oh!

evansiroky left a comment

Choose a reason for hiding this comment

Uh oh!

leonardehrenfried commented Aug 27, 2019

Uh oh!

abyrd commented Aug 28, 2019

Uh oh!

leonardehrenfried commented Sep 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

t2gran commented Sep 13, 2019

Uh oh!

abyrd commented Sep 13, 2019

Uh oh!

abyrd commented Sep 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

t2gran commented Sep 13, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

leonardehrenfried commented Aug 16, 2019 •

edited

Loading

leonardehrenfried commented Aug 20, 2019 •

edited

Loading

leonardehrenfried commented Sep 5, 2019 •

edited

Loading

abyrd commented Sep 13, 2019 •

edited

Loading