🧠 Implement "uncertainty adapter" within Concentrate #346

bfauble · 2024-03-21T20:31:30Z

Summary of changes

Asana Ticket: 🧠 Implement "uncertainty adapter" within Concentrate

Update to handle update_type field being present at the TripUpdate level from TripUpdates_enhanced.json feed from RTR.

"mid_trip" == 60
"at_terminal" == 120
"reverse_trip" == 360

%{
  "trip" => %{
    "trip_id" => "trip",
    "route_id" => "route"
  },
  "update_type" => "mid_trip",
  "stop_time_update" => [%{
    "arrival" => %{"time" => 100, "uncertainty" => 60},
    "departure" => %{"time" => 200, "uncertainty" => 60}
  }]
}

To see the specific tasks where the Asana app for GitHub is being used, see below:
- https://app.asana.com/0/0/1206695607194179

…rmine uncertainty value

github-actions · 2024-03-21T20:33:42Z

Coverage of commit `afeb8f5`

Summary coverage rate:
  lines......: 92.9% (1407 of 1514 lines)
  functions..: 80.7% (455 of 564 functions)
  branches...: no data found

Files changed coverage rate:
                                                                    |Lines       |Functions  |Branches    
  Filename                                                          |Rate     Num|Rate    Num|Rate     Num
  ========================================================================================================
  lib/concentrate/parser/gtfs_realtime_enhanced.ex                  |96.6%     88| 100%    18|    -      0

Download coverage report

github-actions · 2024-03-21T20:37:03Z

Coverage of commit `282e28e`

Summary coverage rate:
  lines......: 92.9% (1407 of 1514 lines)
  functions..: 80.7% (455 of 564 functions)
  branches...: no data found

Files changed coverage rate:
                                                                    |Lines       |Functions  |Branches    
  Filename                                                          |Rate     Num|Rate    Num|Rate     Num
  ========================================================================================================
  lib/concentrate/parser/gtfs_realtime_enhanced.ex                  |96.6%     88| 100%    18|    -      0

Download coverage report

paulswartz

question: is the parser the right place to be making a change to the data? how is this going to work in the future when we want to incorporate other sources of data into these uncertainty updates?

I'm also a little confused about why this particular change is happening in Concentrate, vs RTR. Since RTR is putting in the update_type value, why isn't it also updating the uncertainty values in the feed it puts out?

bfauble · 2024-03-22T14:15:48Z

question: is the parser the right place to be making a change to the data? how is this going to work in the future when we want to incorporate other sources of data into these uncertainty updates?

I'm also a little confused about why this particular change is happening in Concentrate, vs RTR. Since RTR is putting in the update_type value, why isn't it also updating the uncertainty values in the feed it puts out?

subsequent PRs which expose these values from concentrate will also remove the integer values so the atom/string values are the only things that will be passed from rtr and out of concentrate. I wanted to leave the parsing of int values at least temporarily so that we don't need to coordinate a double deploy of rtr and concentrate to deploy/rollback these changes

bfauble · 2024-03-22T14:25:22Z

@paulswartz to clarify. the example in the description is more about demoing the logic of using update_type in place of the uncertainty values when present. There shouldn't be any case where RTR is publishing an update_type that doesn't match the uncertainty values.

bklebe

@bfauble Thanks for this change! A couple questions:

I'm a little confused about the implementation plan here. Why are we adding update_type to RTR only to convert it back into an opaque uncertainty value in Concentrate?
Do you think this would be better as a GroupFilter than a parser extension? That's what I was imagining, though seeing as the implementation plan here is also different from what I expected, maybe putting it in the parser makes sense.

Let's discuss at standup :)

bklebe

As discussed in Slack, this needs updated now that RTR is providing the update_type field.

paulswartz

suggestion: given the rationale for the ticket, I don't think this can live in the parser.

[...] this would allow [...] for Concentrate to source data from multiple sources to assign and adjust uncertainty values.

and

Will be able to ingest multiple types of inputs to make adjustments to uncertainty in the future - this will only be used to make uncertainty changes based on OIO-indicated sign headway mode initially, but we'll be looking to make more substantial use of this in the future.

If it lives in the parser, there's no way for it to consume multiple sources of data.

github-actions · 2024-04-05T20:16:04Z

Coverage of commit `c27deeb`

Summary coverage rate:
  lines......: 92.7% (1404 of 1515 lines)
  functions..: 80.3% (457 of 569 functions)
  branches...: no data found

Files changed coverage rate:
                                                                    |Lines       |Functions  |Branches    
  Filename                                                          |Rate     Num|Rate    Num|Rate     Num
  ========================================================================================================
  lib/concentrate/parser/gtfs_realtime_enhanced.ex                  |97.7%     86| 100%    18|    -      0

Download coverage report

bfauble · 2024-04-05T20:18:01Z

suggestion: given the rationale for the ticket, I don't think this can live in the parser.

[...] this would allow [...] for Concentrate to source data from multiple sources to assign and adjust uncertainty values.

and

Will be able to ingest multiple types of inputs to make adjustments to uncertainty in the future - this will only be used to make uncertainty changes based on OIO-indicated sign headway mode initially, but we'll be looking to make more substantial use of this in the future.

If it lives in the parser, there's no way for it to consume multiple sources of data.

do you mean the update_type -> uncertainty calculation? If so, along with the exposing of update_type in the enhanced feed, I can also move that calculation to the feed generation step to use the update_type there, but that portion is coming in a followup PR.

paulswartz · 2024-04-05T20:22:03Z

@bklebe mentioned using a GroupFilter, which seems like the appropriate place to me as well.

github-actions · 2024-04-08T20:46:52Z

Coverage of commit `081b6fb`

Summary coverage rate:
  lines......: 92.8% (1414 of 1524 lines)
  functions..: 80.7% (463 of 574 functions)
  branches...: no data found

Files changed coverage rate:
                                                                    |Lines       |Functions  |Branches    
  Filename                                                          |Rate     Num|Rate    Num|Rate     Num
  ========================================================================================================
  lib/concentrate/encoder/gtfs_realtime_helpers.ex                  |98.1%     54|94.4%    18|    -      0
  lib/concentrate/encoder/trip_updates_enhanced.ex                  | 100%      5| 100%     4|    -      0
  lib/concentrate/group_filter/uncertainty_value.ex                 |80.0%     10| 100%     3|    -      0
  lib/concentrate/parser/gtfs_realtime_enhanced.ex                  |97.5%     81| 100%    17|    -      0
  lib/concentrate/trip_descriptor.ex                                |82.6%     23|73.0%    37|    -      0

Download coverage report

bfauble · 2024-04-08T20:46:54Z

@paulswartz updated. This now also includes surfacing the value in the feed: https://app.asana.com/0/505721188639414/1206695607194179/f

paulswartz · 2024-04-09T15:32:35Z

@bfauble I don't see the update_type value in the dev Concentrate feed after the deploy.

paulswartz · 2024-04-09T15:38:51Z

lib/concentrate/group_filter/uncertainty_value.ex

+    Enum.map(stus, fn stu ->
+      StopTimeUpdate.update_uncertainty(stu, calculate_uncertainty(update_type))
+    end)


suggestion: since calculate_uncertainty only depends on the update_type you shouldn't calculate it within the map.

paulswartz · 2024-04-09T15:39:30Z

lib/concentrate/group_filter/uncertainty_value.ex

+
+  defp set_uncertainty(update_type, stus) do
+    Enum.map(stus, fn stu ->
+      StopTimeUpdate.update_uncertainty(stu, calculate_uncertainty(update_type))


question: this looks like it always sets the uncertainty even if update_type isn't set. won't this override the uncertainty we get from non-RTR sources?

lib/concentrate/trip_descriptor.ex

paulswartz · 2024-04-09T15:42:18Z

test/concentrate/group_filter/unceratinty_value_test.exs

+             end)
+    end
+
+    test "populates uncertainty in TripDescriptor based on update_type value of other" do


suggestion: can you add a test than an uncertainty value without an update_type is kept?

paulswartz · 2024-04-09T15:43:15Z

config/config.exs

@@ -73,7 +73,8 @@ config :concentrate,
    Concentrate.GroupFilter.VehicleAtSkippedStop,
    Concentrate.GroupFilter.VehicleStopMatch,
    Concentrate.GroupFilter.SkippedStopOnAddedTrip,
-    Concentrate.GroupFilter.TripDescriptorTimestamp
+    Concentrate.GroupFilter.TripDescriptorTimestamp,
+    Concentrate.GropuFilter.UncertaintyValue


suggestion: typo, should be Concentrate.GroupFilter.UncertaintyValue

lib/concentrate/encoder/gtfs_realtime_helpers.ex

github-actions · 2024-04-09T15:52:55Z

Coverage of commit `64339d8`

Summary coverage rate:
  lines......: 92.8% (1414 of 1524 lines)
  functions..: 80.7% (463 of 574 functions)
  branches...: no data found

Files changed coverage rate:
                                                                    |Lines       |Functions  |Branches    
  Filename                                                          |Rate     Num|Rate    Num|Rate     Num
  ========================================================================================================
  lib/concentrate/encoder/gtfs_realtime_helpers.ex                  |98.1%     54|94.4%    18|    -      0
  lib/concentrate/encoder/trip_updates_enhanced.ex                  | 100%      5| 100%     4|    -      0
  lib/concentrate/group_filter/uncertainty_value.ex                 |80.0%     10| 100%     3|    -      0
  lib/concentrate/parser/gtfs_realtime_enhanced.ex                  |97.5%     81| 100%    17|    -      0
  lib/concentrate/trip_descriptor.ex                                |82.6%     23|73.0%    37|    -      0

Download coverage report

…certainty value if update_type is not present

github-actions · 2024-04-24T17:13:29Z

Coverage of commit `0a57eb1`

Summary coverage rate:
  lines......: 92.9% (1416 of 1525 lines)
  functions..: 80.7% (463 of 574 functions)
  branches...: no data found

Files changed coverage rate:
                                                                    |Lines       |Functions  |Branches    
  Filename                                                          |Rate     Num|Rate    Num|Rate     Num
  ========================================================================================================
  lib/concentrate/encoder/gtfs_realtime_helpers.ex                  |98.1%     54|94.4%    18|    -      0
  lib/concentrate/encoder/trip_updates_enhanced.ex                  | 100%      5| 100%     4|    -      0
  lib/concentrate/group_filter/uncertainty_value.ex                 |90.0%     10| 100%     3|    -      0
  lib/concentrate/parser/gtfs_realtime_enhanced.ex                  |97.6%     82| 100%    17|    -      0
  lib/concentrate/trip_descriptor.ex                                |82.6%     23|73.0%    37|    -      0

Download coverage report

paulswartz

Definitely looks like there's a performance drop with this (graph of scheduler usage):

Let me know if you need help looking into it!

github-actions · 2024-04-25T21:07:02Z

Coverage of commit `5f789ea`

Summary coverage rate:
  lines......: 92.5% (1411 of 1525 lines)
  functions..: 80.5% (462 of 574 functions)
  branches...: no data found

Files changed coverage rate:
                                                                    |Lines       |Functions  |Branches    
  Filename                                                          |Rate     Num|Rate    Num|Rate     Num
  ========================================================================================================
  lib/concentrate/encoder/gtfs_realtime_helpers.ex                  |98.1%     54|94.4%    18|    -      0
  lib/concentrate/encoder/trip_updates_enhanced.ex                  | 100%      5| 100%     4|    -      0
  lib/concentrate/group_filter/uncertainty_value.ex                 |80.0%     10| 100%     3|    -      0
  lib/concentrate/parser/gtfs_realtime_enhanced.ex                  |97.6%     82| 100%    17|    -      0
  lib/concentrate/trip_descriptor.ex                                |82.6%     23|73.0%    37|    -      0

Download coverage report

github-actions · 2024-04-25T21:21:00Z

Coverage of commit `151d844`

Summary coverage rate:
  lines......: 92.8% (1415 of 1525 lines)
  functions..: 80.7% (463 of 574 functions)
  branches...: no data found

Files changed coverage rate:
                                                                    |Lines       |Functions  |Branches    
  Filename                                                          |Rate     Num|Rate    Num|Rate     Num
  ========================================================================================================
  lib/concentrate/encoder/gtfs_realtime_helpers.ex                  |98.1%     54|94.4%    18|    -      0
  lib/concentrate/encoder/trip_updates_enhanced.ex                  | 100%      5| 100%     4|    -      0
  lib/concentrate/group_filter/uncertainty_value.ex                 |80.0%     10| 100%     3|    -      0
  lib/concentrate/parser/gtfs_realtime_enhanced.ex                  |97.6%     82| 100%    17|    -      0
  lib/concentrate/trip_descriptor.ex                                |82.6%     23|73.0%    37|    -      0

Download coverage report

This reverts commit 151d844.

github-actions · 2024-04-25T21:37:07Z

Coverage of commit `e175528`

Summary coverage rate:
  lines......: 92.8% (1415 of 1525 lines)
  functions..: 80.7% (463 of 574 functions)
  branches...: no data found

Files changed coverage rate:
                                                                    |Lines       |Functions  |Branches    
  Filename                                                          |Rate     Num|Rate    Num|Rate     Num
  ========================================================================================================
  lib/concentrate/encoder/gtfs_realtime_helpers.ex                  |98.1%     54|94.4%    18|    -      0
  lib/concentrate/encoder/trip_updates_enhanced.ex                  | 100%      5| 100%     4|    -      0
  lib/concentrate/group_filter/uncertainty_value.ex                 |80.0%     10| 100%     3|    -      0
  lib/concentrate/parser/gtfs_realtime_enhanced.ex                  |97.6%     82| 100%    17|    -      0
  lib/concentrate/trip_descriptor.ex                                |82.6%     23|73.0%    37|    -      0

Download coverage report

github-actions · 2024-04-25T21:39:17Z

Coverage of commit `e175528`

Summary coverage rate:
  lines......: 92.8% (1415 of 1525 lines)
  functions..: 80.7% (463 of 574 functions)
  branches...: no data found

Files changed coverage rate:
                                                                    |Lines       |Functions  |Branches    
  Filename                                                          |Rate     Num|Rate    Num|Rate     Num
  ========================================================================================================
  lib/concentrate/encoder/gtfs_realtime_helpers.ex                  |98.1%     54|94.4%    18|    -      0
  lib/concentrate/encoder/trip_updates_enhanced.ex                  | 100%      5| 100%     4|    -      0
  lib/concentrate/group_filter/uncertainty_value.ex                 |80.0%     10| 100%     3|    -      0
  lib/concentrate/parser/gtfs_realtime_enhanced.ex                  |97.6%     82| 100%    17|    -      0
  lib/concentrate/trip_descriptor.ex                                |82.6%     23|73.0%    37|    -      0

Download coverage report

paulswartz

Performance looks better now!

update realtime_enhanced parser to use update_type if present to dete…

afeb8f5

…rmine uncertainty value

bfauble requested review from a team, dks-mbta and bklebe and removed request for a team and dks-mbta March 21, 2024 20:31

reverse_trip not reverse_prediction

282e28e

paulswartz reviewed Mar 22, 2024

View reviewed changes

bklebe reviewed Mar 25, 2024

View reviewed changes

fsaid90 requested a review from bklebe March 26, 2024 14:11

bklebe approved these changes Apr 5, 2024

View reviewed changes

bklebe suggested changes Apr 5, 2024

View reviewed changes

remove fallback parsing of uncertainty if update_type not present

0d56d09

bfauble requested a review from bklebe April 5, 2024 20:02

Merge branch 'master' into bf-uncertainty-adapter

6fab52d

paulswartz reviewed Apr 5, 2024

View reviewed changes

format

c27deeb

move uncertainty calculation to group filter

081b6fb

bklebe approved these changes Apr 9, 2024

View reviewed changes

bfauble temporarily deployed to dev April 9, 2024 14:04 — with GitHub Actions Inactive

paulswartz requested changes Apr 9, 2024

View reviewed changes

fix typo

64339d8

bfauble temporarily deployed to dev April 9, 2024 16:29 — with GitHub Actions Inactive

fix update_type not being passed along to feed and don't overwrite un…

75e5c67

…certainty value if update_type is not present

bfauble requested a review from paulswartz April 24, 2024 17:02

fix trip_update test setup

0a57eb1

bfauble temporarily deployed to dev April 24, 2024 18:31 — with GitHub Actions Inactive

paulswartz reviewed Apr 25, 2024

View reviewed changes

only run group filter if update type is binary

5f789ea

bfauble temporarily deployed to dev April 25, 2024 21:06 — with GitHub Actions Inactive

TEMP

151d844

bfauble temporarily deployed to dev April 25, 2024 21:19 — with GitHub Actions Inactive

bfauble added 2 commits April 25, 2024 15:34

Revert "TEMP"

85e238b

This reverts commit 151d844.

is_nil instead of is_binary

e175528

bfauble temporarily deployed to dev April 25, 2024 21:36 — with GitHub Actions Inactive

bfauble temporarily deployed to dev-green April 25, 2024 21:53 — with GitHub Actions Inactive

bfauble temporarily deployed to dev April 25, 2024 22:07 — with GitHub Actions Inactive

bfauble requested a review from paulswartz April 26, 2024 14:08

paulswartz approved these changes Apr 26, 2024

View reviewed changes

bfauble merged commit 206ebfc into master Apr 26, 2024
18 checks passed

bfauble deleted the bf-uncertainty-adapter branch April 26, 2024 17:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🧠 Implement "uncertainty adapter" within Concentrate #346

🧠 Implement "uncertainty adapter" within Concentrate #346

bfauble commented Mar 21, 2024 •

edited

Loading

github-actions bot commented Mar 21, 2024

github-actions bot commented Mar 21, 2024

paulswartz left a comment

bfauble commented Mar 22, 2024

bfauble commented Mar 22, 2024

bklebe left a comment

bklebe left a comment

paulswartz left a comment •

edited

Loading

github-actions bot commented Apr 5, 2024

bfauble commented Apr 5, 2024

paulswartz commented Apr 5, 2024

github-actions bot commented Apr 8, 2024

bfauble commented Apr 8, 2024

paulswartz commented Apr 9, 2024

paulswartz Apr 9, 2024

paulswartz Apr 9, 2024

paulswartz Apr 9, 2024

paulswartz Apr 9, 2024

github-actions bot commented Apr 9, 2024

github-actions bot commented Apr 24, 2024

paulswartz left a comment •

edited

Loading

github-actions bot commented Apr 25, 2024

github-actions bot commented Apr 25, 2024

github-actions bot commented Apr 25, 2024

github-actions bot commented Apr 25, 2024

paulswartz left a comment

🧠 Implement "uncertainty adapter" within Concentrate #346

🧠 Implement "uncertainty adapter" within Concentrate #346

Conversation

bfauble commented Mar 21, 2024 • edited Loading

Summary of changes

github-actions bot commented Mar 21, 2024

Coverage of commit afeb8f5

github-actions bot commented Mar 21, 2024

Coverage of commit 282e28e

paulswartz left a comment

Choose a reason for hiding this comment

bfauble commented Mar 22, 2024

bfauble commented Mar 22, 2024

bklebe left a comment

Choose a reason for hiding this comment

bklebe left a comment

Choose a reason for hiding this comment

paulswartz left a comment • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Apr 5, 2024

Coverage of commit c27deeb

bfauble commented Apr 5, 2024

paulswartz commented Apr 5, 2024

github-actions bot commented Apr 8, 2024

Coverage of commit 081b6fb

bfauble commented Apr 8, 2024

paulswartz commented Apr 9, 2024

paulswartz Apr 9, 2024

Choose a reason for hiding this comment

paulswartz Apr 9, 2024

Choose a reason for hiding this comment

paulswartz Apr 9, 2024

Choose a reason for hiding this comment

paulswartz Apr 9, 2024

Choose a reason for hiding this comment

github-actions bot commented Apr 9, 2024

Coverage of commit 64339d8

github-actions bot commented Apr 24, 2024

Coverage of commit 0a57eb1

paulswartz left a comment • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Apr 25, 2024

Coverage of commit 5f789ea

github-actions bot commented Apr 25, 2024

Coverage of commit 151d844

github-actions bot commented Apr 25, 2024

Coverage of commit e175528

github-actions bot commented Apr 25, 2024

Coverage of commit e175528

paulswartz left a comment

Choose a reason for hiding this comment

bfauble commented Mar 21, 2024 •

edited

Loading

Coverage of commit `afeb8f5`

Coverage of commit `282e28e`

paulswartz left a comment •

edited

Loading

Coverage of commit `c27deeb`

Coverage of commit `081b6fb`

Coverage of commit `64339d8`

Coverage of commit `0a57eb1`

paulswartz left a comment •

edited

Loading

Coverage of commit `5f789ea`

Coverage of commit `151d844`

Coverage of commit `e175528`

Coverage of commit `e175528`