Skip to content

feat(bus): Keep original values for gtfs_arrival_dt and gtfs_departure_dt - #795

Open
runkelcorey wants to merge 13 commits into
mainfrom
feat-bus-untreat-arrival-time
Open

runkelcorey wants to merge 13 commits into
mainfrom
feat-bus-untreat-arrival-time

Conversation

@runkelcorey

Copy link
Copy Markdown
Collaborator

Working group needs to be able to reconstruct the stop_arrival_dt and stop_departure_dt from its components.

What changes does this PR propose?

Refactors to defer creating stop_arrival_dt and stop_departure_dt until the last stage.

How were these changes validated?

  1. Moved and updated tests
  2. Ran bus runner locally

What questions should reviewers consider?

None.

@runkelcorey
runkelcorey requested a review from a team as a code owner August 18, 2026 21:41
@runkelcorey
runkelcorey requested a review from huangh August 18, 2026 21:41
@github-actions

Copy link
Copy Markdown

LCOV of commit 0cf1d44 during Continuous Integration (Python) #2066

Summary coverage rate:
  lines......: 64.6% (3606 of 5582 lines)
  functions..: 29.9% (276 of 924 functions)
  branches...: no data found

Files changed coverage rate:
                                                                                     |Lines       |Functions  |Branches    
  Filename                                                                           |Rate     Num|Rate    Num|Rate     Num
  =========================================================================================================================
  src/lamp_py/bus_performance_manager/events_gtfs_rt.py                              |85.5%     76|44.4%    18|    -      0
  src/lamp_py/bus_performance_manager/events_joined.py                               |76.3%     59|28.6%    14|    -      0
  src/lamp_py/bus_performance_manager/events_metrics.py                              |68.9%     61|35.7%    14|    -      0
  src/lamp_py/runtime_utils/remote_files.py                                          | 100%     78|50.0%     6|    -      0

Comment thread src/lamp_py/bus_performance_manager/events_metrics.py
Copilot AI lite review requested due to automatic review settings September 10, 2026 14:40

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

read_vehicle_positions can return an unbound local after an exception, which can mask the real read failure and break the pipeline.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR updates the bus performance pipeline to preserve original GTFS-RT timestamp components (gtfs_arrival_dt / gtfs_departure_dt) by deferring derivation of stop_arrival_dt / stop_departure_dt until the metrics stage, and adjusts validation + tests accordingly.

Changes:

  • Stop overwriting GTFS arrival timestamps during the RT↔schedule join; derive stop_arrival_dt/stop_departure_dt later in metrics.
  • Move/adjust validation for “arrival required when GTFS-RT data exists” from BusEvents to BusPerformanceMetrics, updating tests.
  • Refactor GTFS-RT parquet reading toward a Polars LazyFrame pipeline and bump the published bus_events S3 version.
File summaries
File Description
tests/bus_performance_manager/test_events_metrics.py Adds/adjusts metrics-level validation tests and updates existing metrics tests for new required columns.
tests/bus_performance_manager/test_events_joined.py Removes the join-stage test for final-stop GTFS arrival validation (logic moved downstream).
src/lamp_py/runtime_utils/remote_files.py Bumps published bus_events dataset version from 1.2 to 1.3.
src/lamp_py/bus_performance_manager/events_metrics.py Adds a new rule (has_arrival_dt) and refines stop arrival/departure derivation to use GTFS in-transit timestamps without mutating originals.
src/lamp_py/bus_performance_manager/events_joined.py Removes join-stage logic that backfilled/overwrote gtfs_arrival_dt using gtfs_last_in_transit_dt.
src/lamp_py/bus_performance_manager/events_gtfs_rt.py Changes parquet ingest to return a LazyFrame, removes the pyarrow fallback, and updates downstream types.
src/lamp_py/bus_performance_manager/init.py Trims trailing whitespace in the module docstring.
Review details

Suppressed comments (3)

src/lamp_py/bus_performance_manager/events_gtfs_rt.py:124

  • In read_vehicle_positions, if _read_with_polars raises, vehicle_positions is never assigned; the function then returns an unbound local, masking the original read error with an UnboundLocalError. Re-raise after logging (and ensure log_complete() still runs).
    try:
        vehicle_positions = _read_with_polars(service_date, gtfs_rt_files, bus_routes)
    except Exception as e:
        logger.log_failure(e)

    logger.log_complete()
    return vehicle_positions

src/lamp_py/bus_performance_manager/events_gtfs_rt.py:43

  • The _read_with_polars docstring still references a pyarrow implementation and has typos (e.g., “enginer”). Since the pyarrow fallback was removed and the function now returns a LazyFrame, the docstring should be updated to match current behavior.

This issue also appears on line 118 of the same file.

    """
    Read RT_VEHICLE_POSITIONS parquet files with polars engine

    Polars engine appears to be faster and use less memory than pyarrow enginer, but is not as
    compatible with all parquet file formats as pyarrow engine
    """

src/lamp_py/bus_performance_manager/events_metrics.py:223

  • This inline comment is incorrect: shift(-1) pulls the value from the next stop (not the previous one). Updating it will prevent confusion when reasoning about how departure times are derived.
                        .shift(-1)
                        .over(partition_by=["trip_id", "tm_pullout_id"], order_by="stop_sequence"),
                    )
                )  # use the first in transit dt from the previous stop)
                .when(pl.col("point_type").eq(pl.lit("end")))  # endpoints
  • Files reviewed: 6/7 changed files
  • Comments generated: 2
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/lamp_py/bus_performance_manager/events_metrics.py
Comment thread tests/bus_performance_manager/test_events_metrics.py Outdated
@github-actions

Copy link
Copy Markdown

LCOV of commit fb484de during Continuous Integration (Python) #2076

Summary coverage rate:
  lines......: 64.6% (3601 of 5572 lines)
  functions..: 29.9% (276 of 922 functions)
  branches...: no data found

Files changed coverage rate:
                                                                                     |Lines       |Functions  |Branches    
  Filename                                                                           |Rate     Num|Rate    Num|Rate     Num
  =========================================================================================================================
  src/lamp_py/bus_performance_manager/events_gtfs_rt.py                              |92.4%     66|50.0%    16|    -      0
  src/lamp_py/bus_performance_manager/events_joined.py                               |76.3%     59|28.6%    14|    -      0
  src/lamp_py/bus_performance_manager/events_metrics.py                              |68.9%     61|35.7%    14|    -      0
  src/lamp_py/runtime_utils/remote_files.py                                          | 100%     78|50.0%     6|    -      0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants