JFK umass mezzanine refactor #661

PaulJKim · 2023-07-20T15:15:39Z

Summary of changes

The JFK/UMass Mezzanine case where the top line (SB) is on headways while the bottom line is on a prediction was not handled in the previous version and would try to page the prediction on the top line while showing paging headways on the bottom line. This would surpass the character limit for the top line.

This PR takes advantage of some of the new code added for the Early AM Predictions suppression to handle this case.

…k-umass-mezzanine-refactor

github-actions · 2023-07-20T15:41:11Z

Coverage of commit `c2bf89b`

Summary coverage rate:
  lines......: 76.4% (1770 of 2316 lines)
  functions..: 76.2% (486 of 638 functions)
  branches...: no data found

Files changed coverage rate:
                                                             |Lines       |Functions  |Branches    
  Filename                                                   |Rate     Num|Rate    Num|Rate     Num
  =================================================================================================
  lib/content/audio/first_train_scheduled.ex                 |75.0%      8|66.7%     6|    -      0
  lib/content/message/custom.ex                              | 100%     12| 100%     6|    -      0
  lib/content/message/early_am/destination_scheduled_time.ex | 0.0%      2| 0.0%     4|    -      0
  lib/content/message/early_am/destination_train.ex          | 0.0%      2| 0.0%     4|    -      0
  lib/content/message/early_am/scheduled_time.ex             | 0.0%      2| 0.0%     4|    -      0
  lib/content/message/generic_paging.ex                      |50.0%      2|50.0%     4|    -      0
  lib/content/message/platform_prediction_bottom.ex          |33.3%      3|50.0%     4|    -      0
  lib/content/utilities.ex                                   |38.5%     65|62.5%     8|    -      0
  lib/pa_ess/utilities.ex                                    |72.3%    195|96.2%    26|    -      0
  lib/signs/utilities/audio.ex                               |87.7%     73| 100%     7|    -      0
  lib/signs/utilities/messages.ex                            |56.4%     55|70.0%    10|    -      0

Download coverage report

github-actions · 2023-07-21T20:24:23Z

Coverage of commit `cd5c903`

Summary coverage rate:
  lines......: 76.5% (1786 of 2335 lines)
  functions..: 76.4% (491 of 643 functions)
  branches...: no data found

Files changed coverage rate:
                                                             |Lines       |Functions  |Branches    
  Filename                                                   |Rate     Num|Rate    Num|Rate     Num
  =================================================================================================
  lib/signs/utilities/messages.ex                            |56.4%     55|70.0%    10|    -      0

Download coverage report

panentheos

See comments, otherwise looks good.

panentheos · 2023-07-25T14:14:32Z

lib/signs/utilities/messages.ex

+          case Signs.Utilities.Predictions.get_messages(predictions, sign) do
+            {%Content.Message.Empty{}, %Content.Message.Empty{}} ->
+              get_headway_or_alert_messages(sign, current_time, alert_status)
+
+            {top_message, %Content.Message.Empty{}} ->
+              {top_message,
+               get_paging_headway_or_alert_messages(sign, current_time, alert_status, :bottom)}
+
+            {%Content.Message.Empty{}, bottom_message} ->
+              if match?(
+                   %Content.Message.Predictions{station_code: "RJFK", zone: "m"},
+                   bottom_message
+                 ) do
+                jfk_umass_headway_paging(bottom_message, sign, current_time, alert_status)
+              else
+                {get_paging_headway_or_alert_messages(sign, current_time, alert_status, :top),
+                 bottom_message}
+              end
+
+            messages ->
+              messages
+          end


Thinking out loud: This logic looks correct, and I think it's in a good state for now, but it's definitely hinting at a different way of framing the problem. E.g. "for each route, generate a prediction/headway/alert message, then lay them out so they fit." (This is more or less what I mentioned before, just reiterating it in context)

Agreed although, I don't think I have as clear of an idea how that different approach would look in practice. I'd like to hear in more detail about how you're envisioning things! Let's chat soon about it

panentheos · 2023-07-25T14:15:24Z

lib/signs/utilities/messages.ex

+    flip? = flip?(messages)
+
+    if flip?,


nit:

Suggested change

flip? = flip?(messages)

if flip?,

if flip?(messages),

I may leave this one for now in as I re-use the flip? variable in a couple more places in the subsequent PR

panentheos · 2023-07-25T16:06:58Z

lib/signs/utilities/messages.ex

+        {%Content.Message.GenericPaging{
+           messages: [
+             # Make zone nil in order to prevent the usual paging platform message
+             %{prediction | zone: nil},


With this prediction message nested inside the generic paging message, the countup? logic won't trigger as usual. We need to expand that logic if we want it to keep working for this case.

Ah great catch, thank you!

@panentheos just pushed some changes to expand the countup logic. Mind taking another quick pass? 🙏

There may be a larger issue here, if I'm reading this correctly: In the JFK special case, there's an implicit flip happening; the non-empty prediction comes from the bottom, but in the full-page rendering is embedded on top. Because the caller of same_content? always compares top-to-top and bottom-to-bottom, we have no way of matching up the corresponding predictions in that case. The upshot is that as we transition in/out of the special case, we might let a count-up slip through. Does that sound right?

I think addressing that would require a substantial refactor of the same_content? calculation, which seems overly risky right now. My inclination is to document this issue with a comment, and address it in a future change. Thoughts?

Ohh hm yes, I think that's correct. We can't match up the predictions when we're transitioning between the two formats and would potentially miss a count-up.

Alternatively, I think we could get around having to do a more substantial refactor by mapping the PlatformPredictionBottom struct to a regular Prediction struct before calling countup? again. I think it should be a safe assumption that the destinations are the same (:alewife) since this is such a specific case, or we could also just pass along the destination when we create the PlatformPredictionBottom struct. We already pass along the minutes to the bottom so we would have all the pieces handy for the countup logic to do its thing.

Yeah, I guess that works, as long as we call out with a comment that it's tailored for the special case.

github-actions · 2023-07-25T18:42:40Z

Coverage of commit `9bd9b14`

Summary coverage rate:
  lines......: 76.4% (1786 of 2338 lines)
  functions..: 76.4% (491 of 643 functions)
  branches...: no data found

Files changed coverage rate:
                                                             |Lines       |Functions  |Branches    
  Filename                                                   |Rate     Num|Rate    Num|Rate     Num
  =================================================================================================
  lib/signs/utilities/messages.ex                            |53.4%     58|70.0%    10|    -      0

Download coverage report

github-actions · 2023-07-25T20:54:22Z

Coverage of commit `94c5f4c`

Summary coverage rate:
  lines......: 76.4% (1786 of 2338 lines)
  functions..: 76.4% (491 of 643 functions)
  branches...: no data found

Files changed coverage rate:
                                                             |Lines       |Functions  |Branches    
  Filename                                                   |Rate     Num|Rate    Num|Rate     Num
  =================================================================================================
  lib/content/message/platform_prediction_bottom.ex          |33.3%      3|50.0%     4|    -      0
  lib/signs/utilities/messages.ex                            |53.4%     58|70.0%    10|    -      0

Download coverage report

PaulJKim added 5 commits July 20, 2023 10:54

Add content changes

e326387

Add kenmore B stops (#659)

e0c02ca

Add other GL routes to Medford branch source configs (#656)

1c3f324

refactor jfk umass mezzanine code

92519d9

Add format_minutes helper

2e4bacf

PaulJKim mentioned this pull request Jul 20, 2023

Early AM Logic #655

Merged

7 tasks

Merge branch 'pk/early-am-predictions-suppression-content' into pk/jf…

c2bf89b

…k-umass-mezzanine-refactor

Merge branch 'main' into pk/jfk-umass-mezzanine-refactor

cd5c903

add additional comment to clarify intent

ff55c59

PaulJKim requested a review from panentheos July 24, 2023 14:06

PaulJKim assigned panentheos Jul 24, 2023

panentheos approved these changes Jul 25, 2023

View reviewed changes

Expand countup? logic to handle generic paging struct

9bd9b14

Fix countup handling logic for JFK/UMass Mezzanine

94c5f4c

PaulJKim merged commit be77d8c into main Jul 26, 2023
1 check passed

PaulJKim deleted the pk/jfk-umass-mezzanine-refactor branch July 26, 2023 13:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JFK umass mezzanine refactor #661

JFK umass mezzanine refactor #661

PaulJKim commented Jul 20, 2023

github-actions bot commented Jul 20, 2023

github-actions bot commented Jul 21, 2023

panentheos left a comment

panentheos Jul 25, 2023

PaulJKim Jul 25, 2023

panentheos Jul 25, 2023

PaulJKim Jul 25, 2023

panentheos Jul 25, 2023

PaulJKim Jul 25, 2023

PaulJKim Jul 25, 2023

panentheos Jul 25, 2023

PaulJKim Jul 25, 2023

panentheos Jul 25, 2023

github-actions bot commented Jul 25, 2023

github-actions bot commented Jul 25, 2023

JFK umass mezzanine refactor #661

JFK umass mezzanine refactor #661

Conversation

PaulJKim commented Jul 20, 2023

Summary of changes

github-actions bot commented Jul 20, 2023

Coverage of commit c2bf89b

github-actions bot commented Jul 21, 2023

Coverage of commit cd5c903

panentheos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jul 25, 2023

Coverage of commit 9bd9b14

github-actions bot commented Jul 25, 2023

Coverage of commit 94c5f4c

Coverage of commit `c2bf89b`

Coverage of commit `cd5c903`

Coverage of commit `9bd9b14`

Coverage of commit `94c5f4c`