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

Update to newest version of GTFS Flex location groups #5655

Merged
merged 14 commits into from
Feb 16, 2024

Conversation

leonardehrenfried
Copy link
Member

@leonardehrenfried leonardehrenfried commented Feb 1, 2024

Summary

In the recent Flex working group meeting we decided to slightly change the semantics of stop locations: google/transit#388 (comment)

The original change in OBA is here: OneBusAway/onebusaway-gtfs-modules#227

Backwards compatibility

Since Arcadis tooling is still outputting the previous iteration of the spec (stop areas), I added some backwards compatibility code for that. Once the spec has been approved I will remove it and other backwards-compatibility code.

GTFS files

You will notice that this PR adds lots of new files to the repo. This actually only unzips the GTFS zips that were there before so no new (slow!) integration test has been added.

Unit tests

Added.

cc @gcamp @westontrillium @tzujenchanmbd @eliasmbd

@leonardehrenfried leonardehrenfried requested a review from a team as a code owner February 1, 2024 17:05
@leonardehrenfried leonardehrenfried added Improvement IBI Developed by or important for IBI Group labels Feb 1, 2024
Copy link

codecov bot commented Feb 1, 2024

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (d79bb93) 67.62% compared to head (edac0d0) 67.64%.
Report is 15 commits behind head on dev-2.x.

Files Patch % Lines
...g/opentripplanner/gtfs/mapping/StopAreaMapper.java 75.00% 3 Missing and 2 partials ⚠️
.../opentripplanner/gtfs/graphbuilder/GtfsModule.java 0.00% 2 Missing and 1 partial ⚠️
...g/opentripplanner/gtfs/mapping/StopTimeMapper.java 50.00% 1 Missing ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##             dev-2.x    #5655      +/-   ##
=============================================
+ Coverage      67.62%   67.64%   +0.02%     
- Complexity     16290    16309      +19     
=============================================
  Files           1886     1887       +1     
  Lines          71542    71579      +37     
  Branches        7384     7390       +6     
=============================================
+ Hits           48378    48418      +40     
+ Misses         20660    20656       -4     
- Partials        2504     2505       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@leonardehrenfried
Copy link
Member Author

I've converted this PR to a draft as Arcadis has requested that some backwards-compatibility remains.

My goal is to support two types of location groups:

  • the very latest version of the spec draft with the de-normalised location groups
  • the version of the spec draft when we used stop areas as location groups (Arcadis tooling still outputs this, but of course they will move to the spec once it's approved)

Since this is quite a confusing topic, can you confirm that this plan is what we agreed, @miles-grant-ibigroup, @br648?

@leonardehrenfried leonardehrenfried marked this pull request as ready for review February 7, 2024 09:36
@br648
Copy link

br648 commented Feb 7, 2024

I've converted this PR to a draft as Arcadis has requested that some backwards-compatibility remains.

My goal is to support two types of location groups:

* the very latest version of the spec draft with the de-normalised location groups

* the version of the spec draft when we used stop areas as location groups (Arcadis tooling still outputs this, but of course they will move to the spec once it's approved)

Since this is quite a confusing topic, can you confirm that this plan is what we agreed, @miles-grant-ibigroup, @br648?

@leonardehrenfried This sound about right to me. The very latest (new location groups) and the last iteration (Area and StopArea). For clarity, data tools is still on the last iteration and will be migrated to the latest once the spec draft has been approved.

Copy link
Contributor

@vpaturet vpaturet left a comment

Choose a reason for hiding this comment

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

only minor remarks

Comment on lines +358 to +360
for (var transferRule : store.getAllEntitiesForType(FareLegRule.class)) {
transferRule.getFareProductId().setAgencyId(reader.getDefaultAgencyId());
}
Copy link
Member

Choose a reason for hiding this comment

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

What is this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

Setting the correct agency id, which we convert to a feed id, was wrong before.

@leonardehrenfried leonardehrenfried merged commit 9d249e5 into opentripplanner:dev-2.x Feb 16, 2024
5 checks passed
@leonardehrenfried leonardehrenfried deleted the flex-upgrade branch February 16, 2024 13:05
t2gran pushed a commit that referenced this pull request Feb 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
IBI Developed by or important for IBI Group Improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants