Better handle shuttle alert directionality#440
Conversation
There was a problem hiding this comment.
This looks good overall, just some minor suggestions and a sanity check about the updated select query.
My understanding of the approach is that direction ID (when present in an informed entity) is now included in the TimeTable ETS select query so that we avoid selecting shuttle data that doesn't apply to a trip in this direction. Does that sound right?
| id: asdf-cache | ||
| # only run `asdf install` if we didn't hit the cache | ||
| - uses: asdf-vm/actions/install@v1 | ||
| - uses: asdf-vm/actions/install@v4 |
There was a problem hiding this comment.
suggestion:
Should the cache key also be updated to avoid using a cached install from the old version of asdf / the asdf install action? Like change -v2- to -v4- in the key, perhaps.
| ] | ||
| for direction_id <- direction_ids(InformedEntity.direction_id(entity)) do | ||
| {{:route_stop, route_id, stop_id, direction_id}, | ||
| shuttle_type(InformedEntity.activities(entity))} |
There was a problem hiding this comment.
suggestion:
Since the value will be the same for all elements of the list, you could evaluate shuttle_type(InformedEntity.activities(entity)) once outside of this comprehension.
(Bit of a micro-optimization tbh, since the list has only 3 elements max)
| case TimeTable.date_overlaps(@table, {:route_stop, route_id, stop_id}, date_or_timestamp) do | ||
| case TimeTable.date_overlaps( | ||
| @table, | ||
| {:route_stop, route_id, stop_id, direction_id}, |
There was a problem hiding this comment.
question:
Does the shuttles timetable ETS table contain both 3-tuples that do not include direction ID, and 4-tuples that do include direction ID?
I guess I'm a little surprised that you didn't need to make any corresponding changes over in the TimeTable module (or here?) for this new record structure.
(I admit I'm a little rusty on ETS MatchSpecs, so I might be misunderstanding how this key ends up getting used in the select query.)
There was a problem hiding this comment.
They now contain only the four-tuples - I didn't directly change the code in your "here" link, but I did change the cancellation_type/1 function that it calls, which is how we generate the tuples that get added to the table.
Correct! |
Co-authored-by: Jon Zimbel <63608771+jzimbel-mbta@users.noreply.github.com>
2 participants
Summary of changes
Asana Ticket: 🧠🐛Trains that run outside the limits of a diversion shouldn't have stops skipped
Inbound Lowell Line trains were originating at
NHRML-0218-01, the normal outbound platform at North Billerica. Because theinformed_entityin the alert (1017626) had activities ofBOARDandRIDE, it was interpreted as the start of a shuttle, and therefore Concentrate skipped it and all of the stop time updates that came after it in the trip (since there was no other stop that counted as the end of the shuttle). However, theinformed_entityalso had adirection_idof 0, indicating that it was only effective in the outbound direction, and Concentrate wasn't respecting this. This change makes Concentrate reference the direction ID in theinformed_entity, if present, to determine what kind of effect the alert is having on a given route and stop. If the direction ID is absent, it is assumed to apply in both directions.Unfortunately this may be a bit hard to test without another very similar shuttle alert, schedule, and realtime tracking information. I'm hoping that the unit tests are sufficient and adequately replicate the scenario that caused the Lowell Line issue.