Wrong date_time for cross midnight trips (GMT offset issue)

[xavierraffin]

En été, en France, nous sommes en temps GMT+2.

Nous avons des services "Noctambus" qui roule de 1h à 4h du matin.
Nous avons découvert une anomalie dans les résultats de routes_schedules avec ces services.

J'ai un peu regardé le code (voir mes idées sur la correction à la fin) et Stephan m'a dit de vous passer le bébé car il y a plein d'effet de bord, notament sur les lignes à fréquence que je voudrais éviter.
Donc j'ai reproduit un use case simplifié dans un NTFS et je vous le décrit ci dessous :

Description de l'anomalie

Contexte

Voici le lien du NTFS utilisé : http://web31srv.tisseo.fr/NTFS_GMT2.zip

En gros il n'y a qu'une ligne, qu'une route avec 3 trips passant sur un unique calendrier WE valable du 1/7/2016 au 31/7/2016.
Il y a seulement 3 arrêts 1, 2 et 3.

Ces trips partent à 0h50, 1h50, et 2h50 (ci dessous les stop_times)

`trip_id,stop_id,stop_sequence,arrival_time,departure_time,stop_headsign,pickup_type,drop_off_type,date_time_estimated
2,1,1,00:50:00,00:50:00,,0,0,1
2,2,2,01:05:00,01:05:00,,0,0,1
2,3,3,01:15:00,01:15:00,,0,0,1
3,1,1,01:50:00,01:50:00,,0,0,1
3,2,2,02:05:00,02:05:00,,0,0,1
3,3,3,02:15:00,02:15:00,,0,0,1
4,1,1,02:50:00,02:50:00,,0,0,1
4,2,2,03:05:00,03:05:00,,0,0,1
4,3,3,03:15:00,03:15:00,,0,0,1

Ce qui se passe

Quand on appelle route_schedules sur l'unique route

http://srv-dev04/v1/coverage/tisseo_fh/routes/route:1_A/route_schedules?from_datetime=20160704&calendar=Y2FsZW5kYXI6OA&show_codes=true

on obtient:

date_times": [
{
    "date_time": "20160702T025000",
},
{
    "date_time": "20160703T005000",
},
{
    "date_time": "20160703T015000",
}

Vous avez bien vu : le service de 2h50 est en premier et les services suivants ont été décalés d'un jour.

Ce qui devrait se passer

On devrait obtenir la réponse suivante

date_times": [
{
    "date_time": "20160702T005000",
},
{
    "date_time": "20160702T015000",
},
{
    "date_time": "20160702T025000",
},

J'espère qu'on est d'accord :-)

Suggestion de correction

J'ai compris que route_schedules avait deux manière différente de répondre suivant qu'on lui passe un calendar ou pas.
Le problème a lieu avec le calendar.

Dans ce contexte, il n'y a pas de référence à de véritables jours.
Or quand le NTFS est lu et enregistré dans ED on stocke le nombre de seconde après minuit + GMT offset

ed_fh=# select vehicle_journey_id, departure_time from stop_time;
 vehicle_journey_id | departure_time
--------------------+----------------
                  0 |          82200
                  1 |          85800
                  2 |           3000

Or à l'import on ne veut pas que ça dépasse 86400 pour le premier stop du trip.
Donc 2h50 su mat devient 3000s au lieu de 89400
Et pour que le calculateur sache bien le jour d'application, il y a un décallage du bitmask du calendrier associé au vehicule_journey (via shift_times de types.h).

Et ensuite quand le calculateur est démarré, on d'afficher les heures sans lire le décallage de bitmask.
Comme on a pas de référence jour (puisqu'on a demandé les passage pour un grid_calendar donné) on se vautre.

J'avais commencé un patch, mais ça ne marche pas et je me suis arrêté après avoir parlé à Stephan.
Mais ça permet de voir ou est le code à toucher :

J'essayait de recaler tous les résultats sur la même date.

diff --git a/source/time_tables/get_stop_times.cpp b/source/time_tables/get_stop_times.cpp
index ac43c79..5324506 100644
--- a/source/time_tables/get_stop_times.cpp
+++ b/source/time_tables/get_stop_times.cpp
@@ -160,12 +160,22 @@ get_all_stop_times(const type::JourneyPatternPoint* jpp,
     }

     std::vector<std::pair<DateTime, const type::StopTime*>> res;
+    boost::gregorian::date first_vj_date;
+    bool is_first_vj = true;
     for (const auto vj: vjs) {
         //loop through stop times for stop jpp->stop_point
         const auto& st = *(vj->stop_time_list.begin() + jpp->order);
         if (! st.vehicle_journey->accessible(vehicle_properties)) {
             continue; //the stop time must be accessible
         }
+       if(is_first_vj) {
+            first_vj_date = vj->validity_pattern->beginning_date;
+            is_first_vj = false;
+        }
         if (st.is_frequency()) {
             //if it is a frequency, we got to expand the timetable

@@ -183,11 +193,17 @@ get_all_stop_times(const type::JourneyPatternPoint* jpp,
                 }

                 //we need to convert this to local there since we do not have a precise date (just a period)
-                res.push_back({time + freq_vj->utc_to_local_offset, &st});
+                if(first_vj_date < vj->validity_pattern->beginning_date)
+                    res.push_back({(time + freq_vj->utc_to_local_offset)%DateTimeUtils::SECONDS_PER_DAY, &st});
+                else
+                    res.push_back({time + freq_vj->utc_to_local_offset, &st});
             }
         } else {
             //same utc tranformation
-            res.push_back({st.departure_time + vj->utc_to_local_offset, &st});
+            if(first_vj_date < vj->validity_pattern->beginning_date)
+                res.push_back({(st.departure_time + vj->utc_to_local_offset)%DateTimeUtils::SECONDS_PER_DAY, &st});
+            else
+                res.push_back({st.departure_time + vj->utc_to_local_offset, &st});
         }
     }

Bon courage à Antoine (je crois que c'est lui qui a gagné le gros lot)

[kinnou02]

Merci!

J'ajoute le lien vers le ticket interne, sinon on va l'oublier: http://jira.canaltp.fr/browse/NAVITIAII-1872

[antoine-de]

Hi @xavierraffin , sorry for the late response but I have one good and one bad news.

The good news is (at least for me 😌 ) there is no UTC bug 😉

The bad news is there is a quite a huge bug in the route schedule API when called with a calendar.
Simply said, we shouldn't have any dates in the result (it does not have any meaning).

For /stop_schedules what we do is:
when called with only a datetime we have the stop_schedule from this datetime:
(Note: all following examples are done with you dataset):

/stop_points/stop_point:2/stop_schedules?from_datetime=20160704T011000

stop_schedules": [{
...
date_times": [
{ "date_time": "20160716T020500",},
{ "date_time": "20160716T030500",},
{ "date_time": "20160717T010500",}
]}]

Note: the default is to limit to 24h, so the 3rd datetime is the next day at 01:05 since we ask for the departure after 01:10

when called with a calendar you don't get date, only time (the date is not relevant since you ask for the schedule for say all weekends):
/stop_points/stop_point:2/stop_schedules?calendar=Y2FsZW5kYXI6OA

stop_schedules": [{
...
date_times": [
{ "date_time": "010500"}
{ "date_time": "020500"}
{  "date_time": "030500"}
]}]

Note: the date_time are sorted compared to midnight

If you want another sort (eg, you want the first date_time to be after 02:00), it's not straight forward, but you can use the same from_datetime parameter:

Note: this is not great because this param is then used quite differently as it's primary purpose, and the API requires it to be a datetime even if only the time will be used. I would be to change the API to accept only time when given a calendar parameter.

/stop_points/stop_point:2/stop_schedules?from_datetime=20160716T020000&calendar=Y2FsZW5kYXI6OA:

stop_schedules": [{
...
date_times": [
{  "date_time": "020500"}
{  "date_time": "030500"}
{  "date_time": "010500"}
]}]

Is this behaviour ok with you ?

I think the /route_schedules API should have the same behaviour, but it is not the case, the date is dumped when a calendar parameter is provided.

Is it ok for you if we change the /route_schedules API ? It will not break the API in the strict sens, but it can break you integration (I don't think it is used with calendar in our side).

Side note: the fact that with your call the datetime are sorted with this order:
/routes/route:1_A/route_schedules?from_datetime=20160704&calendar=Y2FsZW5kYXI6OA&show_codes=true

date_times": [
{    "date_time": "20160702T025000",},
{    "date_time": "20160703T005000",},
{    "date_time": "20160703T015000",}

is due to the fact that you do not provide an our in you parameter from_datetime=20160704, thus 00:00 is considered.
This date is then wrongly changed in UTC (this is the behaviour we want when no calendar is provided) to 02:00), thus you get first 0250, then 0050 0150
we'll need to stop shifting the from_datetime param to UTC when a calendar is provided since in that case, only local time is relevant.

[xavierraffin]

Is this behaviour ok with you ?

That's OK for me.
Lot of good ideas and intentions here.
Nevertheless changing from "YYYMMddThhmmss" to "YYYMMdd" is a little bit misleading.
Date format consistence may be more important than date insignificance (Stephan agree last month ) ?
Note that in that case I understand why, I just ask for my "navitia culture".
You'll stay like that for v2 api ?

Note: the default is to limit to 24h

Is there a parameter to increase it ?

Is it ok for you if we change the /route_schedules API ?

Yes that sounds good but I have few questions

• Does it means that you'll change "date_time" format from "YYYMMddThhmmss" to "YYYMMdd" ?
• if I understand well, youre correction consist only to stop the UTC shift when a calendar is given to route_schedules ?
  I mean are you sure it will be enough ?

I tried to add an hour on my request (version 1.20), and nothing changes

http://srv-dev04/v1/coverage/tisseo_fh/routes/route:1_A/route_schedules?from_datetime=20160704T033000&calendar=Y2FsZW5kYXI6OA&show_codes=true

return the same than

http://srv-dev04/v1/coverage/tisseo_fh/routes/route:1_A/route_schedules?from_datetime=20160704&calendar=Y2FsZW5kYXI6OA&show_codes=true

date_times": [
{    "date_time": "20160702T025000",},
{    "date_time": "20160703T005000",},
{    "date_time": "20160703T015000",}

[antoine-de]

Nevertheless changing from "YYYMMddThhmmss" to "YYYMMdd" is a little bit misleading.
Date format consistence may be more important than date insignificance (Stephan agree last month ) ?
Note that in that case I understand why, I just ask for my "navitia culture".
You'll stay like that for v2 api ?

hum no we'll change from YYYMMddThhmmss to hhmmss (or Thhmmss I need to check the standard), but only for calendar's route_schedules
and I do agree that it does not seems straight forward, I think in the v2, we'll split it in 2 fields, one optional date and the time.

Is there a parameter to increase it ?

yes duration

Yes that sounds good but I have few questions
Does it means that you'll change "date_time" format from "YYYMMddThhmmss" to "YYYMMdd" ?

no we'll change it to hhmmss

if I understand well, youre correction consist only to stop the UTC shift when a calendar is given to route_schedules ?
I mean are you sure it will be enough ?

No the correction is a bit more complex, we need to change the /route_schedules API behaviour to be more consistent the the /stop_schedule's:

• refacto the api's route_schedules.cpp file to do like /stop_schedules
• change date_time format
• do not shift from_datetime parameter in UTC if a calendar is provided

I'm not 100% sure it will be enough, I need to test, but if the stop_schedule behaviour is OK with you, I think it will be ok.

I tried to add an hour on my request (version 1.20), and nothing changes

strange, I'll look into it, but it is not that surprising, since I think the route_schedule.cpp code is not correct to do exactly like stop_schedule.cpp

[xavierraffin]

Great !
Good luck

Yes that sounds good but I have few questions
Does it means that you'll change "date_time" format from "YYYMMddThhmmss" to "YYYMMdd" ?

no we'll change it to hhmmss

Of course (copy paste error)