conveyal / gtfs-lib

A library for loading and saving GTFS feeds of arbitrary size with disk-backed storage
BSD 2-Clause "Simplified" License
71 stars 38 forks source link

Flex table additions #335

Closed br648 closed 2 years ago

br648 commented 3 years ago

Checklist

Description

Update to include the following additional normal CSV tables:

Update to include flex specific fields:

Update to unpack and pack GeoJson data:

GeoJson can handle the following geometry types:

miles-grant-ibigroup commented 2 years ago

Have made a first pass at some of the smaller comments that relate to my work. Thanks for the thorough review Binh!

binh-dam-ibigroup commented 2 years ago

As discussed in teams, this new issue has emerged: Code from #348 and the current DT flex-editor-updates branch produce fixed stop stop_times that are invalid according to code from #347. To reproduce:

  1. Using code from #348, add a fixed stop to a flex pattern in the GTFS editor
  2. Take an editor snapshot and download the snapshot
  3. Using code from #347, create a new feed source and upload the downloaded snapshot
  4. Observe validation errors regarding the mean/safe duration values.

Sample stop_times.txt file produced: the entry for stop yz85 that was added to a route pattern should have blank values for the mean/safe duration fields (and possibly the continuous duration/pickup fields too):

trip_id,stop_sequence,stop_id,arrival_time,departure_time,stop_headsign,pickup_type,drop_off_type,continuous_pickup,continuous_drop_off,shape_dist_traveled,timepoint,pickup_booking_rule_id,drop_off_booking_rule_id,start_pickup_dropoff_window,end_pickup_dropoff_window,mean_duration_factor,mean_duration_offset,safe_duration_factor,safe_duration_offset
06671d42-4558-42fb-ad0f-00680b157ab7,0,7y7t,17:30:00,17:30:00,,,,,,,0,,,,,,,,
06671d42-4558-42fb-ad0f-00680b157ab7,1,yz85,,,,0,0,1,1,2157.043671030327,,,,,,0.00,0.00,0.00,0.00
06671d42-4558-42fb-ad0f-00680b157ab7,2,zone_2,,,,2,2,,,,0,1,1,17:30:00,18:00:00,,,,
06671d42-4558-42fb-ad0f-00680b157ab7,3,cujv,18:00:00,18:00:00,,,,,,,0,,,,,,,,
binh-dam-ibigroup commented 2 years ago

As discussed in teams, this new issue has emerged: Code from #348 and the current DT flex-editor-updates branch produce fixed stop stop_times that are invalid according to code from #347. To reproduce:

  1. Using code from Pattern Reconciliation #348, add a fixed stop to a flex pattern in the GTFS editor
  2. Take an editor snapshot and download the snapshot
  3. Using code from Flex validation #347, create a new feed source and upload the downloaded snapshot
  4. Observe validation errors regarding the mean/safe duration values.

Sample stop_times.txt file produced: the entry for stop yz85 that was added to a route pattern should have blank values for the mean/safe duration fields (and possibly the continuous duration/pickup fields too):

trip_id,stop_sequence,stop_id,arrival_time,departure_time,stop_headsign,pickup_type,drop_off_type,continuous_pickup,continuous_drop_off,shape_dist_traveled,timepoint,pickup_booking_rule_id,drop_off_booking_rule_id,start_pickup_dropoff_window,end_pickup_dropoff_window,mean_duration_factor,mean_duration_offset,safe_duration_factor,safe_duration_offset
06671d42-4558-42fb-ad0f-00680b157ab7,0,7y7t,17:30:00,17:30:00,,,,,,,0,,,,,,,,
06671d42-4558-42fb-ad0f-00680b157ab7,1,yz85,,,,0,0,1,1,2157.043671030327,,,,,,0.00,0.00,0.00,0.00
06671d42-4558-42fb-ad0f-00680b157ab7,2,zone_2,,,,2,2,,,,0,1,1,17:30:00,18:00:00,,,,
06671d42-4558-42fb-ad0f-00680b157ab7,3,cujv,18:00:00,18:00:00,,,,,,,0,,,,,,,,

This issue seems to have been resolved, which is nice! The only thing that remains is that values for pickup/dropoff types and continuous pickup/dropoff availability are written in the GTFS stop_times tables, even when they are the default value and could have been omitted (see screenshot below). @br648 you're welcome to do the change in this PR, or, if you decide to merge, I will note this in a new issue.

image
br648 commented 2 years ago

@binh-dam-ibigroup Thanks for the review! I'll address https://github.com/conveyal/gtfs-lib/pull/335#issuecomment-1116169140 in here to save noting a new issue.

br648 commented 2 years ago

@binh-dam-ibigroup The values for pickup/dropoff types and continuous pickup/dropoff are provided by the UI, e.g. :

"pattern_stops":[{"continuous_drop_off":1,"continuous_pickup":1,"default_dwell_time":0,"default_travel_time":0,"drop_off_booking_rule_id":null,"drop_off_type":0,"id":"ca65","pickup_booking_rule_id":null,"pickup_type":0,"shape_dist_traveled":null,"stop_id":"2615682","stop_sequence":2,"timepoint":null}]

image

I think the value of zero (Regularly Scheduled) for pickup/dropoff types is valid.

I'm not so sure about the continuous pickup/dropoff value of one. This perhaps should NOT be provided which would result in the values being omitted from the stop_times tables e.g. this in PatternStop:

public int continuous_pickup = INT_MISSING; public int continuous_drop_off = INT_MISSING;

@miles-grant-ibigroup do you have any thoughts on this?

binh-dam-ibigroup commented 2 years ago

@binh-dam-ibigroup The values for pickup/dropoff types and continuous pickup/dropoff are provided by the UI, e.g. :

"pattern_stops":[{"continuous_drop_off":1,"continuous_pickup":1,"default_dwell_time":0,"default_travel_time":0,"drop_off_booking_rule_id":null,"drop_off_type":0,"id":"ca65","pickup_booking_rule_id":null,"pickup_type":0,"shape_dist_traveled":null,"stop_id":"2615682","stop_sequence":2,"timepoint":null}]

image

I think the value of zero (Regularly Scheduled) for pickup/dropoff types is valid.

I'm not so sure about the continuous pickup/dropoff value of one. This perhaps should NOT be provided which would result in the values being omitted from the stop_times tables e.g. this in PatternStop:

public int continuous_pickup = INT_MISSING; public int continuous_drop_off = INT_MISSING;

@miles-grant-ibigroup do you have any thoughts on this?

This is an existing behavior anyway, right? So I think we can address that in a separate enhancement PR.

br648 commented 2 years ago

@binh-dam-ibigroup The values for pickup/dropoff types and continuous pickup/dropoff are provided by the UI, e.g. : "pattern_stops":[{"continuous_drop_off":1,"continuous_pickup":1,"default_dwell_time":0,"default_travel_time":0,"drop_off_booking_rule_id":null,"drop_off_type":0,"id":"ca65","pickup_booking_rule_id":null,"pickup_type":0,"shape_dist_traveled":null,"stop_id":"2615682","stop_sequence":2,"timepoint":null}] image I think the value of zero (Regularly Scheduled) for pickup/dropoff types is valid. I'm not so sure about the continuous pickup/dropoff value of one. This perhaps should NOT be provided which would result in the values being omitted from the stop_times tables e.g. this in PatternStop: public int continuous_pickup = INT_MISSING; public int continuous_drop_off = INT_MISSING; @miles-grant-ibigroup do you have any thoughts on this?

This is an existing behavior anyway, right? So I think we can address that in a separate enhancement PR.

I think so yes. I'll go ahead and merge.

miles-grant-ibigroup commented 2 years ago

Sorry this slipped off my radar entirely! A lot going on right now.

I agree we can table this for another PR, in fact I believe @philip-cline has already done some relevant work here!