add valhalla's time options by nilsnolde · Pull Request #1 · khamaileon/routingpy

nilsnolde · 2023-08-06T18:47:43Z

Sorry, I got carried away a little bit. Seems I gave bit of a lousy review when adding OTP, there's been a few inconsistencies with other routers, which I patched here as well. Ideally I'd have separated that entirely in a different PR, sorry for a bit of a mess.. Maybe I should've also waited until we merged your PR, then PR valhalla separately. Anyways, let me know if you strongly prefer another way and you can just cherry-pick what you want from here for your PR and I'll deal with valhalla after we merged routingpy#116 for OTP & Google only.

I mainly wanted to make the review back and forth a bit easier and have smth tangible to show what I mean.Let me know smth looks weird or is not clear, I'm not 100% I didn't mess anything up.

nilsnolde · 2023-08-06T18:49:14Z

 geopandas = {version = "^0.8.2", optional = true}
 descartes = {version = "^1.0.0", optional = true}
 pytz = "^2023.3"
+timezonefinder = "^6.2.0"


for valhalla, which (unfortunately) doesn't return a tz aware date_time string

nilsnolde · 2023-08-06T18:53:13Z

        units: Optional[str] = None,
-        arrival_time: Optional[int] = None,
-        departure_time: Optional[int] = None,
+        date_time: Optional[datetime.datetime] = datetime.datetime.now(datetime.timezone.utc),


I made them all more consistent in terms of time arguments:

date_time is everywhere supposed to be a tz-aware date_time object, defaulting to UTC

date_time_type is either depart_at or arrive_by

I put those arguments wherever time is needed, but sometimes they're not doing anything, e.g. OTP isochrones only support depart_at scenarios

We could've done a enum or so, but I thought with a docstring it's ok, and it's one less thing to import for others

Great, I'd planned to tackle that later too! However, I would have preferred a "departure_datetime" and an "arrival_datetime". I find it more concise and more explicit for the user.

I'm open to that. As we're breaking stuff right now, we're flexible. And I kinda agree.

I'll change it here before touching the tests.

nilsnolde · 2023-08-06T18:57:57Z

                plan(
-                    date: "{ date.strftime("%Y-%m-%d") }"
-                    time: "{ time.strftime("%H:%M:%S") }"
+                    date: "{ date_time.date().strftime("%Y-%m-%d") }"


this should do the same as before, just omitting the timezone

Yes, we must validate this in the unit tests.

nilsnolde · 2023-08-06T19:01:35Z

-            arrival_datetime = self._timestamp_to_utc_datetime(itinerary["endTime"])
+            distance, geometry = OpenTripPlannerV2._parse_legs(itinerary["legs"])
+            departure_datetime = convert.timestamp_to_tz_datetime(itinerary["startTime"], "UTC")
+            arrival_datetime = convert.timestamp_to_tz_datetime(itinerary["endTime"], "UTC")


this I don't really understand @khamaileon : does that mean that in the OTP result there's no info which timezone the destination (or departure) is referring to? Is there no way to get the timezone info back from OTP? If not, I'd suggest to find the timezone similar to valhalla with timezonefinder, otherwise anyone using this has the burden to look up the timezone to get the local time.

I think we can consider the input timezone as the output timezone. That's why I insisted on a datetime aware input.

But how does OTP expect it? It expects a UTC date & time in the request right? Then these outputs here by OTP is UTC as well? If we can't find out the timezone, we shouldn't assume the same as the input, it might've changed from origin to destination. I couldn't find the docs for OTP request parameters though..

It expects a local date & time in the request. But we ask the user for an aware datetime so we can use the same timezone to transform the output. Maybe I'm wrong, but I find it hard to imagine a user requesting an itinerary on one timezone with a result on another timezone.

BTW, there is a way to retrieve the timezone for transit legs via agency/timezone.

I find it hard to imagine a user requesting an itinerary on one timezone with a result on another timezone.

That's actually quite common for car or long-distance transit (e.g. Amtrak crossing the US).

BTW, there is a way to retrieve the timezone for transit legs via agency/timezone.

I think that's not granular enough. We'd need the stops.txt/stop_timezone (if OTP exposes that) and I wouldn't be sure that's consistently handled by all operators who indeed have stops in different timezones.

FWIW, I asked a OTP core dev offline (Cc @leonardehrenfried), maybe he has some more insight/recommendations.

Haha he sent me your matrix questions and his answers. So it's the same situation for OTP as we have in Valhalla. It's a rare situation that one operator has timezone-crossing stops, but still it's common to cross timezone in one itinerary. I'd use the same approach for now as in Valhalla and look it up dynamically by its location with timezonefinder. Apparently they want to change it in OTP similar to what I'd like in Valhalla, so this would hopefully only be a temp solution. Ok for you?

That's actually quite common for car or long-distance transit (e.g. Amtrak crossing the US).

Yeah, I see. I think I was too focused on public transport where you only have one timezone per network.

I think that's not granular enough. We'd need the stops.txt/stop_timezone (if OTP exposes that) and I wouldn't be sure that's consistently handled by all operators who indeed have stops in different timezones.

OTP exposes timezone a the stop level, but maybe that's too much information to bring up.

FWIW, I asked a OTP core dev offline (Cc @leonardehrenfried), maybe he has some more insight/recommendations.

Lol I had asked the question on the OTP chat and it was him who answered me!

Yes I'm ok with it! Don't forget timezonefinder has a cost (near 1s / 10000 query)

Come to think of it, now that we won't be using the input timezone for output, do we still need an input datetime aware? And what happens if we enter a departure timezone that isn't the one at the point of origin? Sorry, I can make the changes if we rollback again.

yeah right, it’s a bit of cost, but I guess it’s drowning well in the OTP/Valhalla response time:)

God I hate datetime stuff! It always the head quite a bit.. I’m not sure about OTP‘s isochrones, but I’m now fairly sure that expects a local time as well (timezone would be meaningless). If that’s the case, all routers only need the local time and we can ditch tz aware input? The output is probably the only thing that should have timezone awareness. Google does it right, OTP & Valhalla hopefully soon too.

nilsnolde · 2023-08-06T19:03:27Z

+        profile: str,
+        intervals: List[int],
+        interval_type: Optional[str] = None,


same here: I changed those to be consistent with other routers

Yes, I was planning to do that in the 2nd phase. I wanted the 1st version to reflect the router's API as closely as possible.

Ah ok, that'd have been fine too, as long as we don't release, which we have already. It's not that bad though, the next release will break the date_time stuff anyways, so we'll need to increment major version.

nilsnolde · 2023-08-06T19:35:30Z

+        # TODO: maybe we can get rid of that in the future, in case we'll have timezone aware strings in the response:
+        #   https://github.com/valhalla/valhalla/issues/4241
+        if origin.get("date_time"):
+            departure_time = timestamp_to_tz_datetime(
+                origin["date_time"], lonlat_to_timezone(origin["lon"], origin["lat"])
+            )


valhalla doesn't return the timezone either. it could do that in the date_time string and I think we should. it won't change any public facing API if we change it in the future, so we can start with this

Same as above. I'd take the input timezone.

Same comment as above:) destination can have a different timezone

nilsnolde · 2023-08-06T19:43:16Z

And last question: do you want to be invited as a maintainer to routingpy? No pressure, I don't mind either way. It just makes collaboration a bit easier, at the least the Github goodies like assigning reviewers (not possible across forks).

nilsnolde · 2023-08-06T19:58:38Z

And I should mention this is missing unit tests for valhalla (I didn't do changes to OTP/Google tests either yet).

khamaileon · 2023-08-07T08:12:37Z

        units: Optional[str] = None,
-        arrival_time: Optional[int] = None,
-        departure_time: Optional[int] = None,
+        date_time: Optional[datetime.datetime] = datetime.datetime.now(datetime.timezone.utc),


Great, I'd planned to tackle that later too! However, I would have preferred a "departure_datetime" and an "arrival_datetime". I find it more concise and more explicit for the user.

khamaileon · 2023-08-07T08:16:03Z

        directions = []
        for route in response["routes"]:
-            duration, distance, geometry, departure_datetime, arrival_datetime = self._parse_legs(
+            duration, distance, geometry, departure_datetime, arrival_datetime = Google.parse_legs(


Maybe it's just me, but it seems a bit odd. But I can live with it and we stay consistent with the rest of the code, which is the most important thing.

khamaileon · 2023-08-07T08:16:56Z

-        arrival_time: Optional[int] = None,
-        departure_time: Optional[int] = None,
+        date_time: Optional[datetime.datetime] = datetime.datetime.now(datetime.timezone.utc),
+        date_time_type: Optional[str] = None,


Same as above.

khamaileon · 2023-08-07T08:19:10Z

-        time: Optional[datetime.time] = datetime.datetime.now().time(),
-        arrive_by: Optional[bool] = False,
+        profile: Optional[str],
+        date_time: Optional[datetime.datetime] = datetime.datetime.now(datetime.timezone.utc),


Same as above.

khamaileon · 2023-08-07T08:21:16Z

                plan(
-                    date: "{ date.strftime("%Y-%m-%d") }"
-                    time: "{ time.strftime("%H:%M:%S") }"
+                    date: "{ date_time.date().strftime("%Y-%m-%d") }"


Yes, we must validate this in the unit tests.

khamaileon · 2023-08-07T08:24:33Z

-            arrival_datetime = self._timestamp_to_utc_datetime(itinerary["endTime"])
+            distance, geometry = OpenTripPlannerV2._parse_legs(itinerary["legs"])
+            departure_datetime = convert.timestamp_to_tz_datetime(itinerary["startTime"], "UTC")
+            arrival_datetime = convert.timestamp_to_tz_datetime(itinerary["endTime"], "UTC")


I think we can consider the input timezone as the output timezone. That's why I insisted on a datetime aware input.

khamaileon · 2023-08-07T08:27:10Z

+        profile: str,
+        intervals: List[int],
+        interval_type: Optional[str] = None,


Yes, I was planning to do that in the 2nd phase. I wanted the 1st version to reflect the router's API as closely as possible.

khamaileon · 2023-08-07T08:35:45Z

+        # TODO: maybe we can get rid of that in the future, in case we'll have timezone aware strings in the response:
+        #   https://github.com/valhalla/valhalla/issues/4241
+        if origin.get("date_time"):
+            departure_time = timestamp_to_tz_datetime(
+                origin["date_time"], lonlat_to_timezone(origin["lon"], origin["lat"])
+            )


Same as above. I'd take the input timezone.

khamaileon · 2023-08-07T08:43:48Z

I'm fine with doing it this way. Apart from the comments I've already made, the tests on Valhalla are missing and the results of the dates also need to be tested. Tell me how you want us to proceed, I should have 1 or 2 hours available today. And I'd be delighted to become a contributor. I had already done a code of this type privately a few years ago. It's unmaintainable on its own. Long live FOSS!

nilsnolde · 2023-08-07T10:12:58Z

Ok cool I'll add you to the repo then, thanks!

I'd do the changes to this PR:

add tests for valhalla
adapt tests for google/otp
move to arrival_datetime & destination_datetime, both being mutually exclusive

Just needs clarification on the OTP /route endpoint and how it handles timezones. Is there a sane doc for OTP API parameters? I struggled to find one.

Sounds ok for you? Next collab will happen with a bit more thought on my side:)

khamaileon · 2023-08-07T10:25:42Z

Ok cool I'll add you to the repo then, thanks!

I'd do the changes to this PR:
* add tests for valhalla

* adapt tests for google/otp

* move to `arrival_datetime` & `destination_datetime`, both being mutually exclusive

Great, let me know if you need help.

Just needs clarification on the OTP /route endpoint and how it handles timezones. Is there a sane doc for OTP API parameters? I struggled to find one.

You can look at the GraphQL doc on a local instance: http://localhost:8081/graphiql

Sounds ok for you? Next collab will happen with a bit more thought on my side:)

No worries, it's nice to share it with you already :)

Co-authored-by: Nils <nilsnolde@proton.me>

nilsnolde · 2023-12-15T21:22:27Z

sorry for the long wait. I finally found the time to take care of this. let me know if it's fine for you.

khamaileon · 2024-01-08T15:37:01Z

Hi @nilsnolde, happy new year :)
Sorry end of the year was intensive for me too. I'll have a look ASAP!

leonardehrenfried · 2024-02-21T16:10:05Z

BTW, times which include the offset (~= time zone) will be coming to OTP: opentripplanner/OpenTripPlanner#5660

nilsnolde · 2024-02-21T17:04:50Z

Great:) forgot to update that we also added it a few weeks ago: valhalla/valhalla#4491. returning both tz string and offset