Skip to content
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

fast travel cleaners tech-debt #180

Closed
ethan-moss opened this issue Oct 16, 2023 · 3 comments
Closed

fast travel cleaners tech-debt #180

ethan-moss opened this issue Oct 16, 2023 · 3 comments
Assignees
Labels
GTFS technical debt A better way is available. Fix later approach has been adopted. wontfix This will not be worked on

Comments

@ethan-moss
Copy link
Collaborator

ethan-moss commented Oct 16, 2023

Fast travel tech-debt raised during PR #125, associated with cleaners.py:

  1. Refactor clean_multiple_stop_fast_travel_warnings() and clean_consecutive_stop_fast_travel_warnings() into a common cleaning function:

    • Methods are similar, just the warning message and impacted tables are different.
  2. filter_gtfs_around_trip() units optional argument should be "km" - or take from gtfs.feed.dist_units <--- probably best approach.

3. Use pd.isna() in filter_gtfs_around_trip() - handles strings, no need to try/except.

4. Add a unit test for _gtfs_defence() in the passing case (i.e. using a GTFS test fixture)

5. For _add_validation_row() add defence to ensure message is only either {"warning", "error"}

6. drop_trip() works with trip_ids that aren't in stop_times. This is OK since no cleaning is done, but it's slightly opaque for public function, maybe we can protect against this? Perhaps raise a warning to say not cleaning since no warnings/no valid trip_ids?

Points 3,4,5 & 6 resolved in #254

  1. Develop a unit test for the default speed bounds return statement (when the route_type is not a SPEED_BOUNDS key):
    • modify the GTFS fixture, perhaps replacing the route_type for a trip with a different value?
@ethan-moss ethan-moss added needs triage technical debt A better way is available. Fix later approach has been adopted. GTFS labels Oct 16, 2023
@CBROWN-ONS CBROWN-ONS self-assigned this Oct 19, 2023
@CBROWN-ONS
Copy link
Collaborator

26/10/2023
I have decided than point 1 won't be implemented. This is because it impacts the structure of the pipeline. Instead the beginning of both functions will be functionalised.

@CBROWN-ONS
Copy link
Collaborator

CBROWN-ONS commented Feb 26, 2024

Points 3, 4, 5, 6 will be completed in a separate PR to #195 (#254)

@r-leyshon
Copy link
Contributor

Migrated to datasciencecampus/assess_gtfs#10

@r-leyshon r-leyshon closed this as not planned Won't fix, can't repro, duplicate, stale Aug 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GTFS technical debt A better way is available. Fix later approach has been adopted. wontfix This will not be worked on
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants