Add Stop Time Interpolation #399

philip-cline · 2023-11-05T19:24:16Z

Checklist

Appropriate branch selected (all PRs must first be merged to dev before they can be merged to master)
Any modified or new methods or classes have helpful JavaDoc and code is thoroughly commented
The description lists all applicable issues this PR seeks to resolve
The description lists any configuration setting(s) that differ from the default settings
All tests and CI builds passing

Description

This PR supports interpolating stop times based on timepoints in the normalizeStopTimesForPattern method.

AdrianaCeric

The code works well under a few test cases! I am not too familiar with Java, but we could probably make the error handling more robust in this function:

AdrianaCeric · 2023-11-08T20:31:28Z

src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java

@@ -755,8 +755,9 @@ private int updateStopTimesForPatternStop(ObjectNode patternStop, int previousTr
     *
     * TODO? add param Set<String> serviceIdFilters service_id values to filter trips on
     */
-    private int updateStopTimesForPatternStops(List<PatternStop> patternStops) throws SQLException {
+    private int updateStopTimesForPatternStops(List<PatternStop> patternStops, boolean interpolateStopTimes) throws SQLException {
        PatternStop firstPatternStop = patternStops.iterator().next();


(Unlikely edge case) Say I accidentally make a pattern with zero stops and click Normalize stop times it throws a NoSuchElemenException and a long error.

Is there a way to check for an empty list and to return a cleaner error message?

I think this is best addressed by preventing the behaviour client side, although I'll defer to other reviewers if they'd like some additional checks

AdrianaCeric · 2023-11-08T20:43:24Z

src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java

+                    timepointNumber++;
+                    previousShapeDistTraveled = currentTimepoint.shape_dist_traveled;
+                    PatternStop nextTimePoint = timepoints.get(timepointNumber);
+                    timepointSpeed = (nextTimePoint.shape_dist_traveled - currentTimepoint.shape_dist_traveled) / nextTimePoint.default_travel_time;


Should we check if default_travel_time can't be zero before doing this division?

Good point, yeah I'll add a case for that

miles-grant-ibigroup

Good enough for now, I'd either add a lot of tests and error cases or a comment that this could use a good refactor

miles-grant-ibigroup · 2023-11-10T18:14:19Z

src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java

+            int timepointNumber = 0;
+            double timepointSpeed = 0;
+            double previousShapeDistTraveled = 0;
+            PatternStop currentTimepoint = patternStops.get(0); // First stop must be a timepoint


I would add a check here to verify that the item exists and is a timepoint

Good call, added in b585c9d

miles-grant-ibigroup · 2023-11-10T18:14:35Z

src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java

+                boolean isTimepoint = patternStop.timepoint == 1;
+                // If we have reached a timepoint (which is not the last), we calculate the speed between it and the next timepoint.
+                if (isTimepoint && interpolateStopTimes && timepointNumber < timepoints.size()-1){
+                    timepointNumber++;


Include the ++ in the if

I was gonna do this, but was worried it made the code less readable.

Updated this in b585c9d

miles-grant-ibigroup · 2023-11-10T18:14:52Z

src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java

+                if (isTimepoint && interpolateStopTimes && timepointNumber < timepoints.size()-1){
+                    timepointNumber++;
+                    previousShapeDistTraveled = currentTimepoint.shape_dist_traveled;
+                    PatternStop nextTimePoint = timepoints.get(timepointNumber);


make sure this isn't null

Done in b585c9d

br648

Initial feedback. If we can keep all updates to JdbcTableWriter in separate methods it might help with managing this class. I might also get you to merge this update into dev-flex!

br648 · 2023-11-17T12:32:27Z

src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java

@@ -788,10 +789,28 @@ private int updateStopTimesForPatternStops(List<PatternStop> patternStops) throw
        final BatchTracker stopTimesTracker = new BatchTracker("stop_times", updateStopTimeStatement);
        for (String tripId : timesForTripIds.keySet()) {
            // Initialize travel time with previous stop time value.
-            int cumulativeTravelTime = timesForTripIds.get(tripId);
+            int cumulativeTravelTime = timesForTripIds.get(tripId), timepointNumber = 0;


Declare timepointNumber on a separate line e.g. int timepointNumber = 0;

Updated in 36f3043

br648 · 2023-11-17T12:32:47Z

src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java

@@ -788,10 +789,28 @@ private int updateStopTimesForPatternStops(List<PatternStop> patternStops) throw
        final BatchTracker stopTimesTracker = new BatchTracker("stop_times", updateStopTimeStatement);
        for (String tripId : timesForTripIds.keySet()) {
            // Initialize travel time with previous stop time value.
-            int cumulativeTravelTime = timesForTripIds.get(tripId);
+            int cumulativeTravelTime = timesForTripIds.get(tripId), timepointNumber = 0;
+            double timepointSpeed = 0, previousShapeDistTraveled = 0;


Declare previousShapeDistTraveled on a separate line.

Updated in 36f3043

br648 · 2023-11-17T12:47:04Z

src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java

            for (PatternStop patternStop : patternStops) {
+                boolean isTimepoint = patternStop.timepoint == 1;
+                // If we have reached a timepoint (which is not the last), we calculate the speed between it and the next timepoint.
+                if (isTimepoint && interpolateStopTimes && timepointNumber++ < timepoints.size()-1){


Instead of injecting the new code into this for loop, look to create a new interpolation method which will carry out the required tasks and return the updated travelTime value.

Updated to use a new method in 36f3043

br648 · 2023-11-17T12:48:10Z

src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java

+                    PatternStop nextTimePoint = timepoints.get(timepointNumber);
+                    if (nextTimePoint == null) throw new IllegalStateException("Stop time interpolation is not possible with null timepoints.");
+                    timepointSpeed = nextTimePoint.default_travel_time == Entity.INT_MISSING ? 0
+                        : (nextTimePoint.shape_dist_traveled - currentTimepoint.shape_dist_traveled) / nextTimePoint.default_travel_time;


Nit. Line up the ? and : makes this easier to read.

Updated in 36f3043

br648 · 2023-11-17T12:52:32Z

src/test/java/com/conveyal/gtfs/loader/JDBCTableWriterTest.java

-    @Test
-    public void canNormalizePatternStopTimes() throws IOException, SQLException, InvalidNamespaceException {
-        // Store Table and Class values for use in test.
+    private static String normalizeStopsForPattern(PatternStopDTO[] patternStops, int updatedStopSequence, boolean interpolateStopTimes, int initialTravelTime, int updatedTravelTime, int startTime, String patternId) throws SQLException, InvalidNamespaceException, IOException {


Reduce the length of this line by putting each param on a new line.

Updated in 36f3043

src/test/java/com/conveyal/gtfs/loader/JDBCTableWriterTest.java

src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java

…ments

br648

This is getting there, the method is much better. I just have some concerns over the impact of default values.

br648 · 2023-11-21T08:22:26Z

src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java

+     */
+    public int normalizeStopTimesForPattern(int id, int beginWithSequence) throws SQLException {
+        return normalizeStopTimesForPattern(id, beginWithSequence, false);
+    }


Nit. New line after closing bracket.

Fixed in dbd09ce

br648 · 2023-11-21T08:36:43Z

src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java

@@ -745,6 +752,33 @@ private int updateStopTimesForPatternStop(ObjectNode patternStop, int previousTr
        return travelTime + dwellTime;
    }

+    private int interpolateTimesFromTimepoints(


Method comment to describe what this is doing.

Added in dbd09ce

br648 · 2023-11-21T08:45:35Z

src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java

+            throw new IllegalStateException("All timepoints must have a default travel time specified.");
+        }
+
+        double timepointSpeed = (nextTimepoint.shape_dist_traveled - lastTimepoint.shape_dist_traveled) / nextTimepoint.default_travel_time;


If shape_dist_traveled is not defined and set to Double.MIN_VALUE, isn't that going to cause issues here?

Good point, I added checks for every pattern stop and both timepoints in dbd09ce

br648 · 2023-11-21T08:49:14Z

src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java

                // Gather travel/dwell time for pattern stop (being sure to check for missing values).
                int travelTime = patternStop.default_travel_time == Entity.INT_MISSING ? 0 : patternStop.default_travel_time;
+                if (!isTimepoint && interpolateStopTimes) {
+                    // Override travel time if we're interpolating between timepoints


Nit. Full stop.

Added in dbd09ce

br648 · 2023-11-21T09:13:51Z

src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java

+                    // Override travel time if we're interpolating between timepoints
+                    travelTime = interpolateTimesFromTimepoints(patternStop, timepoints, timepointNumber, previousShapeDistTraveled);
+                }
+                previousShapeDistTraveled += patternStop.shape_dist_traveled;


If any shape_dist_traveled is set to Double.MIN_VALUE, will that cause an issue?

likewise yes, I added a check here too dbd09ce

br648 · 2023-11-21T09:55:51Z

src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java

+                if (!isTimepoint && interpolateStopTimes) {
+                    // We don't want to increment the true cumulative travel time because that adjusts the timepoint
+                    // times later in the pattern.
+                    // TODO? We ignore dwell times in interpolation calculations right now.


TODOs have a habit of not being done. Can dwell times be incorporated easily now? If not, explain why they are ignored and remove the TODO.

I added an explanation of why this is left out, and an explanation of how it could be implemented. Right now there's not much reason to since it doesn't fit the use case which is driving this feature.

br648

LGTM.

philip-cline added 2 commits November 5, 2023 14:10

feat(JdbcTableWriter): add stoptime interpolation

1e2adad

refactor(JdbcTableWriter): update tests

5f34e04

This was referenced Nov 5, 2023

Add Stop Time Interpolation ibi-group/datatools-ui#998

Merged

Support Stop Time Interpolation ibi-group/datatools-server#572

Merged

philip-cline assigned miles-grant-ibigroup Nov 6, 2023

AdrianaCeric approved these changes Nov 8, 2023

View reviewed changes

philip-cline assigned br648 and unassigned miles-grant-ibigroup Nov 10, 2023

miles-grant-ibigroup approved these changes Nov 10, 2023

View reviewed changes

miles-grant-ibigroup self-requested a review November 10, 2023 18:16

refactor(JDBCTableWriter): add tests, respond to comments

b585c9d

philip-cline assigned miles-grant-ibigroup Nov 13, 2023

refactor(JDBCTableWriter): don't share pattern IDs

36ccb09

br648 removed their assignment Nov 14, 2023

philip-cline assigned br648 and miles-grant-ibigroup and unassigned miles-grant-ibigroup and br648 Nov 14, 2023

miles-grant-ibigroup assigned philip-cline and unassigned miles-grant-ibigroup Nov 16, 2023

philip-cline assigned br648 and miles-grant-ibigroup and unassigned philip-cline and miles-grant-ibigroup Nov 16, 2023

br648 requested changes Nov 17, 2023

View reviewed changes

br648 assigned philip-cline and unassigned br648 Nov 17, 2023

philip-cline added 2 commits November 17, 2023 14:01

refactor(JdbcTableWriter): move interpolation into separate method

8a1fe2a

refactor(st interpolation): Update calculation method, respond to com…

36f3043

…ments

philip-cline assigned br648 Nov 20, 2023

philip-cline removed their assignment Nov 20, 2023

br648 requested changes Nov 21, 2023

View reviewed changes

br648 assigned philip-cline and unassigned br648 Nov 21, 2023

refactor(st interpolation): respond to PR comments

dbd09ce

philip-cline assigned br648 and unassigned philip-cline Nov 21, 2023

philip-cline added 3 commits November 21, 2023 11:40

refactor(st interpolation): update illegalState text

da9cc6b

refactor(st interpolation): update formatting

5fd655d

refactor(st interpolation): updating method description

a331d74

br648 approved these changes Nov 21, 2023

View reviewed changes

br648 assigned philip-cline and unassigned br648 Nov 21, 2023

philip-cline merged commit b657dfe into dev Nov 21, 2023
2 checks passed

Add Stop Time Interpolation #399

Add Stop Time Interpolation #399

Conversation

philip-cline commented Nov 5, 2023 • edited Loading

Checklist

Description

AdrianaCeric 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

miles-grant-ibigroup 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

br648 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

br648 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

br648 left a comment

Choose a reason for hiding this comment

philip-cline commented Nov 5, 2023 •

edited

Loading