No date_times for stop schedule on first stop of loop line

[eturck]

While testing stop schedules on a more stupid complex line I found a problem when there is a loop. If you want an example here it is on navitia.io This is Tisséo's line 111 : It's a tiny loop. And the first and last stop_points are the same.

When I ask for stop_schedules on this stop_point, I expect to see all the departures of the loop but instead date_times is empty. It's pretty clear as to why, because when we render the result in departure_boards :

for(auto dt_st : id_vec.second) {
    const auto& st_calendar = navitia::StopTimeCalendar(dt_st.second, dt_st.first, calendar_id);
    // terminus or partial terminus
    if (is_last_stoptime(st_calendar.stop_time, stop_point)) {
        continue;
    }
    auto date_time = schedule->add_date_times();
    (...)
}

If the stop_point is equal to the last stop_point of the vehicle_journey we don't output the stop_times. Which is a problem when the line is a loop.

Removing this 'if' doesn't have much effect for 'standard' lines because we just get pick_up stop_time :

stop_times = routing::get_stop_times(routing::StopEvent::pick_up, (...));

And in ed::Data::complete pick_up_drop_of_on_borders is called :

void Data::pick_up_drop_of_on_borders() {
    for (auto* vj: vehicle_journeys) {
        if (vj->stop_time_list.empty()) { continue; }
        if (! vj->prev_vj) {
            vj->stop_time_list.front()->drop_off_allowed = false;
        }
        if (! vj->next_vj) {
            vj->stop_time_list.back()->pick_up_allowed = false;
        }
    }
}

So standard terminuses don't have any pickup stop_times because pick_up isn't allowed on the last stop_time. But this could be a problem for :

• blocks of vj with prev and next vjs. I guess we would display date_times at the terminus but is it a problem if pickup is allowed? I really don't know much about block_ids. I hope you can help with this :)
• calendar request where I didn't see any filter on pick_up_allowed. I might have missed it, it might be intended or it may be a separate issue?

I tried it and the only broke test is on one departure_board integration test with calendars. Which seems to confirm there is an issue with calendar request if the 'if' is removed.

What do you think about all of this? Do you think this would break anything else?

[pbougue]

Looks like it was done in that PR : https://github.com/CanalTP/navitia/pull/1878 So yes, linked to calendars and we should probably be a bit smarter, but the lower the better for code-pooling. To me it shouldn't break anything else, but we should find a way to maintain the behavior of calendar, of course.

[eturck]

To sum up what we said on tuesday the problem is more the way things are tagged terminus or partial terminus than really not outputing stop_times on terminus. What is currently done:

1. If the stop_point of the current route_point is equal to the stop_point of the last stop_time of the vj associated to the stop_time, we don't output this stop_time (done in render)
2. We tag a schedule as "terminus" if the stop_point of the route_point is in the same stop_area as the main destination of the route (in departure_board) (it's taking precedence over other response status)
3. We tag a schedule as "partial_terminus" if EVERY stop_times is terminus of their associated vj (same rule as 1, comparison of the stop_point) and it's not in the same stop_area as the main destination (just below 2).

I looked at PR #1878 and what changed is that before as long as ANY stop_time was terminus all the schedule was tagged "partial_terminus" (here)

What I am trying right now, which would help handle loops:

1. The computation of "if a stop_time is at the terminus" is not done on the stop_point but on the order of the stop_time compared to the order of the last stop_time of the vj,
2. Tag as "terminus" if it's in the same stop_area but also if it's the last stop_time (big impact, see below),
3. Tag as "partial_terminus" if it's the last stop_time and it's not a "terminus".

To move the check for terminus to a simple comparison of stop_area to check on all stop_times is not simple (at least without calendar_id). Because when we ask for next departures without calendar we explicitly ask for StopEvent::pick_up stop_times to next_stop_times. And because we are at a terminus, there is no stop_times returned. I did kind of the same thing than what is done for active_disruption. I request StopEvent::drop_off stop_times if I have no stop_times and no calendar_id is passed in parameters. From there I check if all stop_times are terminus of their vj. If it's the main destination it's a terminus, otherwise a partial_terminus.

All the tests are still passing (yeah :smiley:). But this changes some things:

• stops on a loop line are not tagged as terminus anymore as long as there is departures,
• "terminus" response status does not take precedence anymore. If there is an active disruption or there is no service this day the respective response status will apply because we will not get any drop_off stop_times either,
• "partial_terminus" would be outputted even if no calendar_id is used. Which is not the case right now (except for stay_in or loop lines) since there is no stop_time returned at a partial_terminus.

If you want to see my WIP on the subject : https://github.com/CanalTP/navitia/compare/dev...Tisseo:wip_terminus_changes

I have added no test right now but if the code seems ok I will be adding a few cases in departure_board_test.

[pbougue]

Wow, that's a hard one to read and understand 😓 Didn't check your work or external links to (old) code. For terminus, working on dropoff seems right, which might come with some problems as it's not the regular use of next_stop_times(), so I guess it needs to be parametrized (to avoid future "corrections" that would fit one use but break the other).

I don't know how we should behave about the terminus stability (no-service or disruptions). To me it seems fine to say that the terminus kind of "moved forward" (even if we don't say where it was). If we are right about the terminus over a possibly long period, it seems fine.

[kinnou02]

I finally took the time to read it!

Your change seems good for me, even better that how it was done before. About the impact, I will summarize them to see if I understood.

• We will not have "terminus" on loop line: good since there isn't really a terminus :)
• We now display partial terminus when called without calendar: good it was probably a bug anyway
• The one I don't really understand, what is this "terminus response status"? If by that you mean than the terminus will be changed by disruption or if schedule vehicle journey aren't the same as "usual" If that so we will have to think a little bit about it, it doesn't sound like a no go for me.

[eturck]

Thanks for taking the time to look at it :). To complete what you understood:

• Yes. We will not have "terminus" at a stop used for departure and arrival. I'm not sure we can say that there isn't really a terminus, since it's a terminus too. Another impact is that if we have no arrivals either, we don't tag as terminus, even if it is... But we will probably take with something else, see last element of the list ;)
• Yes, I guess it was a bug, I didn't see many tests on this.
• I'll reword the last one to try to make it clearer!

Response status is the name of the variable we set in the source :). It can take multiple values "active_disruption", "no_departure_this_day", "terminus", "no_active_circulation_this_day", "partial_terminus". Currently, the first check we make is if we are in the same stop_area of the main destination, and if so, we tag the stop as terminus. No other check is done afterward. So "terminus" has the priority over every other status. With the changes I made that's not the case anymore. If there is an active disruption, we could have no departures and no arrivals, so my new terminus check will be false. In that case the response status will be "active_disruption" and not "terminus". It's still true, it's just not the same thing anymore.

[kinnou02]

I'm ok with that