feat: OL Crowding Heuristics Implementation #1879
Conversation
cmaddox5
requested review from
a team and
hannahpurcell
and removed request for
a team
Before reviewing, I have questions on the PR description: do we need to calculate the dwell time? It's just the departure time minus 10 seconds. Also, we don't want to show the widget in this case. We want to log to Splunk to say that "heuristic #1 was met" and include the crowding level at that time. Then we'll make a Splunk dashboard to answer these questions:
I remember there being 2 heuristics to evaluate, and it seems like this PR only captures one. The second one is: two back-to-back readings of crowding are identical. This might be TMI at this point, but rewriting the problem this way helped it click for me. We're trying to figure out if we we can have high confidence crowding predictions earlier than just "once the train has left a station". Can we have high confidence predictions when:
Edit: Wanted to add, the task should have been written with more and clearer detail!! The task description absolutely did not capture this work well. |
This is definitely a case of me overthinking what we needed... That does seem exactly right so unless I run into problems with that, I will make that change.
Guess I got too excited trying to add the widget to the screen more often 😅 . Easy to fix though.
This reminds me that I had a concern about this heuristic at the time and completely forgot to communicate it. Are these for two consecutive screen refreshes or some other interval to be done in the background? Both I feel pose different challenges. Two consecutive screen refreshes seems like most screens will never have a chance to catch that (although if we are just logging, not really a big issue. Just may not be a heuristic most stations use). A background interval isn't really a problem, but will mean I need to spin up a different background process to track it. |
Ah, I don't remember. Maybe ask Paul H? We decided during that heuristics meeting and I cannot at all remember which way we decided |
Coverage of commit
|
|
Added an additional heuristic for consecutive crowing classes. Heuristic logic now live in their own function |
2 consecutive screen refreshes. The time-based heuristic will always get priority just so we aren't forever looking for 2 consecutive classes that are the same. Each refresh that still isn't currently past that time will compare to the previous. If it's the same, log it. Otherwise, replace what is in the cache with the new prediction. |
Coverage of commit
|
|
After our chat in Slack, I agree that these heuristics should be independent. That has been changed. |
Coverage of commit
|
| is_nil(next_train_prediction) or | ||
| alert_makes_this_a_terminal or | ||
| next_train_prediction.vehicle.carriages == [] -> | ||
| [] |
There was a problem hiding this comment.
since get_instance is its own function, this can all be done with pattern matching in the function def. Optional suggestion
| cached_prediction, | ||
| common_params | ||
| ) do | ||
| show_widget_after_dt = DateTime.add(cached_prediction.departure_time, -10) |
There was a problem hiding this comment.
Should maybe call this var show_widget_before_dt
There was a problem hiding this comment.
Well now needs to be >= this dt. I added a comment to this var that should help. Let me know if it's still unclear.
There was a problem hiding this comment.
I do think this value should be called something else, because it suggests we're logging after departure time, but really we're logging before departure time; that is, 10s before departure time. now needs to be later than that time, which is still before the vehicle has even departed.
TL;DR the value represented by this variable is pre-departure, whether by 10s, 15s, or more.
| :gt | ||
| ] do | ||
| log_crowding_info( | ||
| :dwell, |
There was a problem hiding this comment.
Maybe adjust this to match the "rebrand" of time-based heuristic. Perhaps :some_time_before_departure
There was a problem hiding this comment.
Good catch. I went with :time_based because that's what I call the heuristic in code. Is that name clear or is it too general?
There was a problem hiding this comment.
I'm on the fence, but I suppose we can leave it for now (until we add another time based heuristic). But there is another comment referring to dwell time on line 284 that could use clarification
Coverage of commit
|
| ) do | ||
| show_widget_after_dt = DateTime.add(cached_prediction.departure_time, -10) | ||
| # cached_prediction.departure_time minus previous_departure_time_cushion is when we expect crowding to be reliable. | ||
| # When now >= this time, show the widget. |
There was a problem hiding this comment.
Still refers to showing the widget. Perhaps instead:
| # When now >= this time, show the widget. | |
| # When now >= this time, log it. |
Coverage of commit
|
* Added departure_time in epoch seconds to response. (#1911) * feature: New widget endpoint for einks (#1909) * Pull last deploy timestamp from config cache, add to data response * Removed unneeded ResponseMapper * Added frontend for bus einks * feat: OL Crowding Heuristics Implementation (#1879) * Added translation to logger. * Started on heuristics work. * Changed what value is saved in Agent. * Fixed logic for NB triptychs. * Tweaked shutdown logic for accuracy logger. * Removed inspect. * Dialyzer. * Changed heuristic to only log, not show widget. * Changed time we use to determine heuristic. * Changed variable name. * Added an additional heuristic for consecutive crowding classes. * Added nil check. * Refactored heuristics so they run independently. * Improved conditional. * Added a log. * Improved comments. * Made hardcoded value a parameter. * Changed log scenario name. * Improved var name. * Credo. * Addressed comments. * feat: Mercury GL E-Ink audio (#1913) * Added audio SSML to screen data response on gl einks with audio configured. * Updated screens_config. * Tweaked data so we always include the key even if there is no audio. * Added audio column to GL & PreFare in admin table (#1916) * Add filter to make sure ID is a number. (#1915) * fix: Skip CSRF protection on /widget POSTs since they really fetch a new HTML page (#1918) * fix: Skip CSRF protection on /widget POSTs since they really fetch a new HTML page * Build browser pipeline atop browser_no_csrf pipeline * Added suppressions for GL Surge. (#1919) * Added suppressions for GL Surge. * Fixed Govt Ctr alert ids. * Added a GL surge suppression at Kenmore. * Alert ID fix. --------- Co-authored-by: Hannah Purcell <69368883+hannahpurcell@users.noreply.github.com> Co-authored-by: Jon Zimbel <63608771+jzimbel-mbta@users.noreply.github.com>
Asana task: Add logging to triptych app - pt 2
A lot of changes here are refactoring so that credo doesn't complain about too many parameters or functions having too many nested conditionals. The meat of this PR is in
get_instance_using_dwell_time. The gist is that we wait for the train to be stopped at the previous station. Once it is stopped, we get the prediction for that particular part of the trip in order to get the predicted dwell time (departure_time - arrival_time). Whennowis >= to that time (minus 10 seconds just to give ourselves a little time before the train is supposed to leave), we want to show the widget. The original logic of the train being in transit to the current station will always take precedent. This is only to be used as an extra effort to show the widget a little sooner.