Filtering non-pickup calls by barslett · Pull Request #2377 · opentripplanner/OpenTripPlanner

barslett · 2016-12-08T16:29:04Z

IndexAPI (and GraphQL) has apparently had no method to filter away calls in StopTimes that have no boarding (piclupType = 1). This change ignores these stops in stopTimesForStop(). If some implementations still need to show these calls, we can of course make it configurable.

Partly replaces PR #2358, which contained too much clutter.

IndexAPI (and GraphQL) has apparently had no method to filter away calls in StopTimes that have no boarding (piclupType = 1). This change ignores these stops in stopTimesForStop(). If some implementations still need to show these calls, we can of course make it configurable. Partly replaces PR opentripplanner#2358, which contained too much clutter.

hannesj · 2016-12-09T09:07:29Z

It would be nice if this would be based on an argument, as we are showing the arriving vehicles differently in the ui.

barslett · 2016-12-09T09:23:08Z

@hannesj I suggest adding a parameter for this to the router-config. An argument would have to be added to two methods in Index API as well as to the GraphQL request structure.

EDIT: Seems that I wrongly have assumed that the router-config can be used for the Index API. Seems it only affects routing requests over the Plan API...

hannesj · 2016-12-16T11:51:30Z

Thanks!

novalis · 2016-12-30T06:18:11Z

                                         @QueryParam("timeRange") @DefaultValue("86400") int timeRange,
-                                         @QueryParam("numberOfDepartures") @DefaultValue("2") int numberOfDepartures) {
+                                         @QueryParam("numberOfDepartures") @DefaultValue("2") int numberOfDepartures,
+    									 @QueryParam("omitNonPickups") boolean omitNonPickups) {


The formatting looks funny here

novalis · 2016-12-30T06:19:31Z

                for (Stop currStop : pattern.stopPattern.stops) {
                    if (currStop == stop) {
-                        if(pattern.stopPattern.pickups[sidx] == pattern.stopPattern.PICKDROP_NONE) continue;
+                        if(omitNonPickups && pattern.stopPattern.pickups[sidx] == pattern.stopPattern.PICKDROP_NONE) continue;


This is a funny variable name -- you really mean stops that allow neither pickups nor dropoffs (I think they might be called "non-stop timepoints"). At any rate, the name "nonPickups" confused me.

No, we are talking about stops that have dropoff, but pickups aren't allowed. In Oslo, this is the case with most region buses when approaching the urban area. They let off passengers on the way, but not taking any new ones on board.

So, the variable name describes a call that is not for pickup. I could rename it omitPickupOnly or something similar.

novalis · 2016-12-30T06:19:44Z

                int sidx = 0;
                for (Stop currStop : pattern.stopPattern.stops) {
                    if (currStop == stop) {
+                        if(pattern.stopPattern.pickups[sidx] == pattern.stopPattern.PICKDROP_NONE) continue;


novalis · 2016-12-30T06:24:27Z

     * @param startTime Start time for the search. Seconds from UNIX epoch
     * @param timeRange Searches forward for timeRange seconds from startTime
     * @param numberOfDepartures Number of departures to fetch per pattern
+     * @param omitNonPickups If departures with pickup restrictions should be included or not


reword: "If stops at which passengers are neither picked up nor dropped off should be omitted".

No, ref my other comment. Dropoffs are allowed here.

Since this is the key to understanding the parameter, I would still like to re-phrase it a little to avoid people misunderstanding it. How about "@PARAM omitNonPickups If true, do not include vehicles that will not pick up passengers."

novalis · 2017-01-02T20:35:58Z

OK, now I understand better. I'm still concerned about what happens at the terminus of a route (which might be marked as PICKDROP_NONE). Do you know what agencies do here?

barslett · 2017-01-02T22:32:06Z

In our data, the call at the terminus is indeed marked as PICKDROP_NONE, and with this filtering it will not be shown in the list of departures, which is our intended behaviour.

I will guess that other agencies who might want to do this filtering, would like to do it both for intermediate no-boarding stops and for termini. Maybe @hannesj has comments?

barslett · 2017-01-16T13:30:59Z

@novalis - do we have any chance of merging this, ref my last comments?

novalis · 2017-01-16T23:00:54Z

I want to merge this, but I don't feel confident that it is really the right thing to do. If I were still running the project, I would probably take it, since that's my philosophy. But because this is such a niche feature (why not just filter the results afterwards?), I don't want to merge it until I hear more from people who are more closely involved. Why not ask on the mailing list and see what they think?

Sorry to be annoying here, and thanks for contributing.

hannesj · 2017-01-17T12:01:41Z

Actually our use case has changed slightly, as we would like to skip the arrivals/non-pickups for terminal views, but show those for individual stops, so this change would be beneficial to us also.

barslett · 2017-01-17T12:15:23Z

@novalis It's impossible to filter on pickdrop afterwards because the attribute itself doesn't exist in the response model. And as this feature is triggered only by a request argument, it will have no impact on other users.

novalis · 2017-01-17T14:39:12Z

OK, but you could put the attribute in the response. If that had been the PR, I would have approved it without worrying.

The issue is not direct impact on other users. The issue is code complexity. If, tomorrow, someone says that they only care about arrival times for dropoff stops, then another parameter needs to get added. Eventually, we have a lot of parameters and code that is hard to read. Since I don't do much day-to-day maintenance on the code, I don't feel confident making this particular decision.

But like I said, feel free to ask on the mailing list. You can get a definitive answer there.

abyrd · 2017-01-17T15:06:43Z

Hey @novalis and @hannesj, actually I'd say no real need to go to the mailing list. I and other developers do see these discussions, I just haven't had time recently to catch up on PRs. @novalis summarizes the situation well, there is the possibility of continuing growth of the API and unmanageable complexity. But for people who are making active use of OTP in production deployments where you've got staff working on the code regularly, I don't really worry to much about this and would like to integrate your contributions. I'll review this and hopefully merge it soon.

hannesj · 2017-01-17T16:37:31Z

@barslett I've added those fields to the GraphQL schema in our fork. I should create a PR for that (and other features added to the GraphQL endpoint) to upstream.

@novalis The idea is that no code for adding/removing data fields is required when using GraphQL, however what we have noticed is that you often want to do some kind of filtering for the data before it is sent to the client, and that needs to be done server-side. In our example we just want to show the next five departures in the popup, and without this filtering we would need to over-request data from the server and filter out those vehicles arriving from that data.

barslett · 2017-01-19T17:49:44Z

Had to push an additional commit, as the logic was missing from a stopTimes method in both IndexAPI and GraphQL.

abyrd

I think we can go ahead and merge this if the comments can be revised slightly to be more clear. It does have the potential to increase code complexity but I trust that you'll assist in maintaining this code since you're relying on it yourselves.

abyrd · 2017-03-14T10:13:17Z

     * @param startTime Start time for the search. Seconds from UNIX epoch
     * @param timeRange Searches forward for timeRange seconds from startTime
     * @param numberOfDepartures Number of departures to fetch per pattern
+     * @param omitNonPickups If departures with pickup restrictions should be included or not


Since this is the key to understanding the parameter, I would still like to re-phrase it a little to avoid people misunderstanding it. How about "@PARAM omitNonPickups If true, do not include vehicles that will not pick up passengers."

barslett · 2017-03-31T13:43:50Z

Sorry for the delay! Have updated the parameter description. However, Travis fails, seems like a general error.

barslett mentioned this pull request Dec 8, 2016

Added headsign attribute for stoptimes in GraphQL+ filtering non-pickup calls #2358

Closed

Introduced argument omitNonPickups in Index API and GraphQL

fdfc2ff

novalis reviewed Dec 30, 2016

View reviewed changes

Corrected indenting of IndexAPI.java

36e320f

Introducing omitNonPickups where it was missing

5d9c2d7

Removed redundant cast

41b7090

abyrd reviewed Mar 14, 2017

View reviewed changes

abyrd added the awaiting response label Mar 14, 2017

Updated parameter description for @omitNonPickups

79b6be7

novalis merged commit 6b77b6e into opentripplanner:master Jun 28, 2017

demory mentioned this pull request Aug 16, 2017

1.2 Release #2483

Closed

Uh oh!

Conversation

barslett commented Dec 8, 2016

Uh oh!

hannesj commented Dec 9, 2016

Uh oh!

barslett commented Dec 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hannesj commented Dec 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

novalis Dec 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

novalis commented Jan 2, 2017

Uh oh!

barslett commented Jan 2, 2017

Uh oh!

barslett commented Jan 16, 2017

Uh oh!

novalis commented Jan 16, 2017

Uh oh!

hannesj commented Jan 17, 2017

Uh oh!

barslett commented Jan 17, 2017

Uh oh!

novalis commented Jan 17, 2017

Uh oh!

abyrd commented Jan 17, 2017

Uh oh!

hannesj commented Jan 17, 2017

Uh oh!

barslett commented Jan 19, 2017

Uh oh!

abyrd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

barslett commented Mar 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

barslett commented Dec 9, 2016 •

edited

Loading

novalis Dec 30, 2016 •

edited

Loading

barslett commented Mar 31, 2017 •

edited

Loading