Skip to content
This repository was archived by the owner on May 27, 2024. It is now read-only.

WIP - Fix #154 start time of freq based trip should be immutable. #336

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

WIP - Fix #154 start time of freq based trip should be immutable. #336

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7f21b52
Implementation of E053 check.
scrudden Sep 15, 2018
95f03b1
Now uses previousFeedMessage parameter rather than a member hashmap.
scrudden Sep 15, 2018
27623ef
Fix NPE. Previous feed is set to null for other tests so need to chec…
scrudden Sep 15, 2018
1e05604
Revisions based on pull request comments from Sean Barbeau.
scrudden Sep 19, 2018
4761991
Added hashmaps for tripupdates.
scrudden Sep 20, 2018
0eb5739
Fix build. Remove import.
scrudden Sep 20, 2018
9b93487
Remove hashmap for current message and add new logic.
scrudden Sep 20, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions RULES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ Rules are declared in the [`ValidationRules` class](https://github.com/CUTR-at-U
| [E050](#E050) | `timestamp` is in the future
| [E051](#E051) | GTFS-rt `stop_sequence` not found in GTFS data
| [E052](#E052) | `vehicle.id` is not unique
| [E053](#E053) | `start_time` for trip has changed

### Table of Warnings

Expand Down Expand Up @@ -717,6 +718,16 @@ From [VehiclePosition.VehicleDescriptor](https://github.com/google/transit/blob/
#### References:
* [`vehicle.id`](https://github.com/google/transit/blob/master/gtfs-realtime/spec/en/reference.md#message-vehicledescriptor)

<a name="E053"/>

### E053 - `start_time` for trip has changed

A frequency based trip should have the same start time in the descriptor.

From [TripUpdate.TripDescriptor](https://github.com/google/transit/blob/master/gtfs-realtime/spec/en/reference.md#message-tripdescriptor) for `start_time`:

>If the trip corresponds to exact_times=0, then its start_time may be arbitrary, and is initially expected to be the first departure of the trip. Once established, the start_time of this frequency-based exact_times=0 trip should be considered immutable, even if the first departure time changes

# Warnings

<a name="W001"/>
Expand Down
4 changes: 4 additions & 0 deletions ...idator-lib/src/main/java/edu/usf/cutr/gtfsrtvalidator/lib/validation/ValidationRules.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,10 @@ public class ValidationRules {
public static final ValidationRule E052 = new ValidationRule("E052", "ERROR", "vehicle.id is not unique",
"Each vehicle should have a unique ID",
"which is used by more than one vehicle in the feed");

public static final ValidationRule E053 = new ValidationRule("E053", "ERROR", "start_time for frequency-based trip changed.",
"start_time for frequency-based trips",
"exact_times=0 must be immutable");

private static List<ValidationRule> mAllRules = new ArrayList<>();

Expand Down
41 changes: 38 additions & 3 deletions ...in/java/edu/usf/cutr/gtfsrtvalidator/lib/validation/rules/FrequencyTypeZeroValidator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@
package edu.usf.cutr.gtfsrtvalidator.lib.validation.rules;

import com.google.transit.realtime.GtfsRealtime;
import com.google.transit.realtime.GtfsRealtime.TripUpdate;

import edu.usf.cutr.gtfsrtvalidator.lib.model.MessageLogModel;
import edu.usf.cutr.gtfsrtvalidator.lib.model.OccurrenceModel;
import edu.usf.cutr.gtfsrtvalidator.lib.model.helper.ErrorListHelperModel;
Expand All @@ -27,6 +29,7 @@
import org.slf4j.LoggerFactory;

import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;

import static edu.usf.cutr.gtfsrtvalidator.lib.validation.ValidationRules.*;
Expand All @@ -41,17 +44,21 @@
public class FrequencyTypeZeroValidator implements FeedEntityValidator {

private static final org.slf4j.Logger _log = LoggerFactory.getLogger(FrequencyTypeZeroValidator.class);




@Override
public List<ErrorListHelperModel> validate(long currentTimeMillis, GtfsMutableDao gtfsData, GtfsMetadata gtfsMetadata, GtfsRealtime.FeedMessage feedMessage, GtfsRealtime.FeedMessage previousFeedMessage, GtfsRealtime.FeedMessage combinedFeedMessage) {
List<OccurrenceModel> errorListE006 = new ArrayList<>();
List<OccurrenceModel> errorListE013 = new ArrayList<>();
List<OccurrenceModel> errorListW005 = new ArrayList<>();
List<OccurrenceModel> errorListE053 = new ArrayList<>();

for (GtfsRealtime.FeedEntity entity : feedMessage.getEntityList()) {
if (entity.hasTripUpdate()) {

GtfsRealtime.TripUpdate tripUpdate = entity.getTripUpdate();

if (gtfsMetadata.getExactTimesZeroTripIds().contains(tripUpdate.getTrip().getTripId())) {
/**
* NOTE - W006 checks for missing trip_ids, because we can't check for that here - we need the trip_id to know if it's exact_times=0
Expand All @@ -75,9 +82,34 @@ public List<ErrorListHelperModel> validate(long currentTimeMillis, GtfsMutableDa
// W005 - Missing vehicle_id in trip_update for frequency-based exact_times = 0
RuleUtils.addOccurrence(W005, "trip_id " + tripUpdate.getTrip().getTripId(), errorListW005, _log);
}
if(previousFeedMessage!=null)
{
for (GtfsRealtime.FeedEntity previousEntity : previousFeedMessage.getEntityList())
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer to avoid the O(n^2) run time complexity that results from the FOR loop here (for each current entity, loop through each previous entity). This could get very expensive when running on a large number of large feeds in parallel, or when running batch validation on archived data.

Instead I think sacrificing some memory to loop through the previous entities and store references to them in a HashMap (vehicle_id -> GtfsRealtime.FeedEntity) before starting the for (GtfsRealtime.FeedEntity entity : feedMessage.getEntityList()) { loop would be my preferred approach. Then, for each current message, check the HashMap to see if a reference is stored for the current vehicle ID, and if so compare the start_times and other data to see if there is an error. This is how the duplicate vehicle_id rule E052 in VehicleValidator is implemented.

Apologies if your previous implementation was like this before we talked on Slack - I may have misunderstood your message.

Copy link
Author

Choose a reason for hiding this comment

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

Reverted to the original way I had done this.

Copy link
Author

@scrudden scrudden Sep 19, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Need to move Hashmap into validate method and populate with previousFeedMessage at the start of the method.

{
if (previousEntity.hasTripUpdate())
{
if(tripUpdate.getVehicle()!=null && previousEntity.getTripUpdate().getVehicle()!=null)
{
if(previousEntity.getTripUpdate().getVehicle().getId().equals(tripUpdate.getVehicle().getId()))
{
if(tripUpdate.getStopTimeUpdateCount()>0 && previousEntity.getTripUpdate().getStopTimeUpdateCount()>0)
{
if(tripUpdate.getStopTimeUpdate(tripUpdate.getStopTimeUpdateCount()-1).getStopSequence()>=previousEntity.getTripUpdate().getStopTimeUpdate(previousEntity.getTripUpdate().getStopTimeUpdateCount()-1).getStopSequence())
Copy link
Member

Choose a reason for hiding this comment

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

This logic seems to work well when you have a set of stop_time_updates in the feed that's less than the total number of records in stop_times.txt for that trip. In other words, when you have predictions for only a subset of the stops for a trip in a TripUpdate. Most of feeds I've seen in the wild fall into this category. The only place I see this falling apart is if the producer provides stop_time_updates for all stops in the trip, which has been advocated as a "strict" feed by Andrew Byrd and Stefan de Konink in their work with OTP. I say we go with this implementation for now and then handle strict feeds later when we have real examples of strict feeds with frequency exact_times=0 trips.

{
// E053 - start time of trip not consistent for trip updates for frequency-based exact_times = 0
if(!tripUpdate.getTrip().getStartTime().equals(previousEntity.getTripUpdate().getTrip().getStartTime()))
{
RuleUtils.addOccurrence(E053, "vehicle_id " + tripUpdate.getVehicle().getId(), errorListE053, _log);
}
}
}
}
}
}
}
}
}
}

Copy link
Member

Choose a reason for hiding this comment

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

Because a VehiclePositions feed can contain a TripDescriptor too, we should also check for this same rule within the below if (entity.hasVehicle) { IF block.

See TripDescriptorValidator.java for examples of how I've factored out the code to methods for rules that are applied to TripUpdates and VehiclePositions entities.

Copy link
Member

Choose a reason for hiding this comment

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

Thinking more, I think you'd need the current_stop_sequence field to be populated in order for this to be checked against a VehiclePosition entity...

if (entity.hasVehicle()) {
GtfsRealtime.VehiclePosition vehiclePosition = entity.getVehicle();
if (vehiclePosition.hasTrip() &&
Expand Down Expand Up @@ -119,6 +151,9 @@ public List<ErrorListHelperModel> validate(long currentTimeMillis, GtfsMutableDa
if (!errorListW005.isEmpty()) {
errors.add(new ErrorListHelperModel(new MessageLogModel(W005), errorListW005));
}
if (!errorListE053.isEmpty()) {
errors.add(new ErrorListHelperModel(new MessageLogModel(E053), errorListE053));
}
return errors;
}
}
2 changes: 1 addition & 1 deletion ...-realtime-validator-lib/src/test/java/edu/usf/cutr/gtfsrtvalidator/lib/test/UtilTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -844,6 +844,6 @@ public void testIsInFuture() {
@Test
public void testGetAllRules() {
List<ValidationRule> rules = ValidationRules.getRules();
assertEquals(61, rules.size());
assertEquals(62, rules.size());
}
}
73 changes: 73 additions & 0 deletions ...test/java/edu/usf/cutr/gtfsrtvalidator/lib/test/rules/FrequencyTypeZeroValidatorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package edu.usf.cutr.gtfsrtvalidator.lib.test.rules;

import com.google.transit.realtime.GtfsRealtime;
import com.google.transit.realtime.GtfsRealtime.FeedMessage;

import edu.usf.cutr.gtfsrtvalidator.lib.model.ValidationRule;
import edu.usf.cutr.gtfsrtvalidator.lib.test.FeedMessageTest;
import edu.usf.cutr.gtfsrtvalidator.lib.test.util.TestUtils;
Expand Down Expand Up @@ -262,4 +264,75 @@ public void testW005() {

clearAndInitRequiredFeedFields();
}
/**
* E053- inconsistent start time in trip descriptor for frequency-based exact_times = 0
*/
@Test
public void testE053()
{
FrequencyTypeZeroValidator frequencyTypeZeroValidator = new FrequencyTypeZeroValidator();
Map<ValidationRule, Integer> expected = new HashMap<>();
FeedMessage[] feedMessages= new FeedMessage[2];
for( int i = 0; i <2;i++)
{
GtfsRealtime.TripDescriptor.Builder tripDescriptorBuilder = GtfsRealtime.TripDescriptor.newBuilder();

tripDescriptorBuilder.setTripId("1");
tripDescriptorBuilder.setStartDate("4-24-2016");
if(i==0)
{
tripDescriptorBuilder.setStartTime("08:00:00AM");
}
if(i==1)
{
tripDescriptorBuilder.setStartTime("09:00:00AM");
}


GtfsRealtime.VehicleDescriptor.Builder vehicleDescriptorBuilder = GtfsRealtime.VehicleDescriptor.newBuilder();

vehicleDescriptorBuilder.setId("1");

vehiclePositionBuilder.setVehicle(vehicleDescriptorBuilder.build());
vehiclePositionBuilder.setTimestamp(TimestampUtils.MIN_POSIX_TIME);
vehiclePositionBuilder.setTrip(tripDescriptorBuilder.build());
vehiclePositionBuilder.setVehicle(vehicleDescriptorBuilder.build());

feedEntityBuilder.setVehicle(vehiclePositionBuilder.build());

feedMessageBuilder.setEntity(0, feedEntityBuilder.build());

tripUpdateBuilder.setVehicle(vehicleDescriptorBuilder.build());
tripUpdateBuilder.setTrip(tripDescriptorBuilder.build());

for(int j=0;j<i+1;j++)
{
GtfsRealtime.TripUpdate.StopTimeUpdate.Builder stopTimeUpdateBuilder = GtfsRealtime.TripUpdate.StopTimeUpdate.newBuilder();

stopTimeUpdateBuilder.setStopId(""+j);
stopTimeUpdateBuilder.setStopSequence(j);

GtfsRealtime.TripUpdate.StopTimeEvent.Builder stopTimeEventBuilder = GtfsRealtime.TripUpdate.StopTimeEvent.newBuilder();
stopTimeUpdateBuilder.setArrival(stopTimeEventBuilder.build());

tripUpdateBuilder.addStopTimeUpdate(stopTimeUpdateBuilder.build());
}

feedEntityBuilder.setTripUpdate(tripUpdateBuilder.build());

// Add vehicle_id to vehicle position - 1 warning
feedEntityBuilder.setVehicle(vehiclePositionBuilder.build());

feedMessageBuilder.setEntity(0, feedEntityBuilder.build());

feedMessages[i]=feedMessageBuilder.build();


}
results = frequencyTypeZeroValidator.validate(TimestampUtils.MIN_POSIX_TIME, bullRunnerGtfs, bullRunnerGtfsMetadata, feedMessages[1], feedMessages[0], null);
expected.put(ValidationRules.E053, 1);
TestUtils.assertResults(expected, results);
expected.clear();
clearAndInitRequiredFeedFields();
}
}