-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Crowding data confidence determination #673
Conversation
Coverage of commit
|
Coverage of commit
|
Coverage of commit
|
Coverage of commit
|
if predictions.crowding_data_confidence == :high do | ||
# TODO: Pass along crowding data classification once available | ||
[ | ||
%Approaching{ | ||
destination: predictions.destination, | ||
trip_id: predictions.trip_id, | ||
platform: predictions.platform, | ||
route_id: predictions.route_id, | ||
new_cars?: predictions.new_cars? | ||
} | ||
] | ||
else | ||
[ | ||
%Approaching{ | ||
destination: predictions.destination, | ||
trip_id: predictions.trip_id, | ||
platform: predictions.platform, | ||
route_id: predictions.route_id, | ||
new_cars?: predictions.new_cars? | ||
} | ||
] | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking out loud: Not sure how this will end up looking in the following PRs but hopefully there's an opportunity to unify some of this logic a bit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep! This does get unified down the line
crowding_data_confidence = | ||
calculate_crowding_data_confidence( | ||
prediction, | ||
Engine.Locations.for_vehicle(prediction.vehicle_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doing this lookup here means we'll end up grabbing location info for every prediction message, every tick, even ones far away or on other lines. Granted, it's an indexed operation, so it may be fast enough, but it's not needed the vast majority of the time. What about storing the vehicle_id
in the Message
, and then performing this calculation only when we actually need the result?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's a good point. I did end up thinking about that later down the line in a later PR and changed the logic so that we only fetch location if it's an Orange Line prediction. Take a look at this logic for do_crowding/1
and let me know what you think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That does improve things. And the lookup should be fast enough, so it's probably not worth worrying about.
lib/engine/locations.ex
Outdated
} | ||
end | ||
|
||
defp status_to_atom(status) do | ||
defp map_multi_carriage_details(multi_carriage_details) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: This might be better named something like parse_carriage_details(carriage_details)
lib/locations/location.ex
Outdated
trip_id: nil, | ||
consist: [] | ||
defmodule Locations do | ||
defmodule CarriageDetails do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a pretty strong pattern of one-module-per-file in non-test code. Perhaps this should live in a new locations/carriage_details.ex
file?
Whoops sorry @panentheos didn't realize I forgot to push the changes from your last two comments. Just did that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
lib/locations/location.ex
Outdated
@@ -14,6 +15,7 @@ defmodule Locations.Location do | |||
timestamp: DateTime.t() | nil, | |||
route_id: String.t() | nil, | |||
trip_id: String.t() | nil, | |||
consist: list() | |||
consist: list(), | |||
multi_carriage_details: list(CarriageDetails.t()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This type needs the fully qualified name now.
Summary of changes
Asana Ticket: π π§βπ€βπ§π "High Confidence" filter logic
This PR begins calculation of crowding data confidence level. High confidence crowding data is currently defined as