Migrate to update_type #863
Conversation
| @@ -372,7 +368,7 @@ defmodule Predictions.PredictionsTest do | |||
| }, _} = get_all(feed_message, @current_time) | |||
| end | |||
|
|
|||
| test "include predictions with low uncertainty" do | |||
| test "include non reverse predictions" do | |||
| reassign_env(:filter_uncertain_predictions?, true) | |||
There was a problem hiding this comment.
drive-by: Could we rename this flag, since we're otherwise removing the concept of "uncertainty" from the code? Maybe filter_reverse_predictions?. (Curious why this is configurable in the first place.)
There was a problem hiding this comment.
Yeah, we should definitely rename it but I was thinking maybe in a small follow-up task or something since we'd want to change that in the devops repo as well. In terms of why it's configurable, I'd guess it's because back then RTS was running on opstech3 and we would often take advantage of flags like this to quickly change up flags to adjust what RTS was doing. It was added in a really old PR a few years back and may not be necessary anymore. #369
There was a problem hiding this comment.
Hm, yeah, maybe more useful initially as a "feature flag" in the time immediately after the feature was added. I think we could outright remove it now, if @panentheos agrees.
There was a problem hiding this comment.
Yeah you're probably right. The feature flag isn't something we'd likely prefer to use in a situation where we want to filter out any sort of prediction type at large anyway since that requires infra to get involved these days. In the more recent past when we've filtered out terminal/reverse predictions on some heavy rail lines, we just introduced a temporary code change and then revert it when predictions were trustworthy again.
There was a problem hiding this comment.
Yep, I don't think we need as a configurable option. Let's remove it!
lib/predictions/predictions.ex
Outdated
| route_id = trip_update["trip"]["route_id"] | ||
| direction_id = trip_update["trip"]["direction_id"] | ||
| trip_id = trip_update["trip"]["trip_id"] | ||
| revenue_trip? = trip_update["trip"]["revenue"] |
There was a problem hiding this comment.
nit: We could inline some of these below to shorten the boilerplate a bit. Alternatively, we could destructure the function param directly, as long as these fields will always be present.
lib/predictions/predictions.ex
Outdated
| defp has_departed?(prediction) do | ||
| prediction.seconds_until_departure && prediction.seconds_until_departure < 0 && | ||
| not prediction.stopped_at_predicted_stop? | ||
| prediction.seconds_until_departure < 0 and not prediction.stopped_at_predicted_stop? |
There was a problem hiding this comment.
nit: Technically this works, because nil < 0 == false according to Elixir's term ordering rules, but I think I prefer the explicit check as a reminder that seconds_until_departure might be nil
| @@ -372,7 +368,7 @@ defmodule Predictions.PredictionsTest do | |||
| }, _} = get_all(feed_message, @current_time) | |||
| end | |||
|
|
|||
| test "include predictions with low uncertainty" do | |||
| test "include non reverse predictions" do | |||
| reassign_env(:filter_uncertain_predictions?, true) | |||
There was a problem hiding this comment.
Yep, I don't think we need as a configurable option. Let's remove it!
test/signs/realtime_test.exs
Outdated
| @@ -1928,7 +1926,8 @@ defmodule Signs.RealtimeTest do | |||
| stopped_at_predicted_stop?: Keyword.get(opts, :stopped_at_predicted_stop, false), | |||
| boarding_status: Keyword.get(opts, :boarding_status), | |||
| revenue_trip?: true, | |||
| vehicle_id: "v1" | |||
| vehicle_id: "v1", | |||
| type: Keyword.get(opts, :prediction_type, :mid_trip) | |||
There was a problem hiding this comment.
nit: Why not name this option type for consistency?
Summary of changes
Asana Ticket: Migrate to Update_type for trip type
RTS currently uses the
uncertaintyfield in order to distinguish different types of predictions (mid trip, terminal, and reverse). This field will eventually be deprecated by the Transit Data team and has been replaced by a new field provided at the trip level calledupdate_type. This PR migrates RTS to using this new field in favor ofuncertaintywhich also made it easier to clean up the predictions feed parsing code a bit as well.Reviewer Checklist
signs.jsonwas changed, there is a follow-up PR tosigns_uito update its dependency on this project