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

expectedTravelTimeMsec in TemporalMatcher is zero (0) #52

Open
nselikoff opened this issue May 18, 2018 · 6 comments
Open

expectedTravelTimeMsec in TemporalMatcher is zero (0) #52

nselikoff opened this issue May 18, 2018 · 6 comments

Comments

@nselikoff
Copy link

nselikoff commented May 18, 2018

I'm getting some unexpected matching results, and I think I've traced it back to expectedTravelTimeMsec being 0. Can you tell me about this block of code (l398 - 412)?

int expectedTravelTimeMsecForward =
TravelTimes.getInstance().expectedTravelTimeBetweenMatches(
vehicleState.getVehicleId(),
previousAvlTime,
previousMatch, spatialMatch);
int expectedTravelTimeMsecBackward =
TravelTimes.getInstance().expectedTravelTimeBetweenMatches(
vehicleState.getVehicleId(),
previousAvlTime,
spatialMatch, previousMatch);
int expectedTravelTimeMsec = Math.min(expectedTravelTimeMsecForward, expectedTravelTimeMsecBackward);
//expectedTravelTimeMsec = expectedTravelTimeMsecForward;

What's the goal with searching forward and backward? expectedTravelTimeMsecBackward is evaluating to 0, which means expectedTravelTimeMsec gets set to 0 for multiple matches, so when the current match is compared to the previous match, it's always considered better.

@scrudden
Copy link
Member

scrudden commented May 19, 2018

HI Nathan,

I have been looking back at the history of this change and from what I can see it is a change I made while implementing arrival predictions for frequency based services. I think it relates to the fact the expectedTravelTimeBetweenMatches return 0 if the stops are in a decreasing sequence. The stops can be in a decreasing sequence for frequency based services, so the expected travel time should not be 0.

This comment is somewhat related in that it says they can be in a different order for frequency based services.

// If the indices are not increasing then can simply return a travel
// time of 0msec. This shouldn't happen so log an error. But only
// log & investigate if a schedule based assignment since for non schedule
// based ones the trip loops back and so you can have match2 < match1.

This change may be an effort to address this for frequency based services, but I think it may not be done correctly.

I have more recently been working with Barefoot to improve the map matching as this (SpatialMatcher +TemporalMatcher) caused me a lot of issues during the trials of the holding method last year. In these recent changes I have removed this particular change, but these may break the frequency based stuff.

Cheers,

Sean.

@scrudden
Copy link
Member

Could you provide an AVL sample and matching GTFS file that demonstrates the issue you are seeing?

@nselikoff
Copy link
Author

Hi Sean,

Thanks for looking at this. When I revert to the old way this was implemented (essentially just looking "forward"), the matching and arrival/departure generation works much better. These are the feeds I'm working with (schedule based service):

GTFS Static: https://data.omnimodal.io/gtfs/sanfordcommunity-fl-us/sanfordcommunity-fl-us.zip
GTFS Realtime (raw vehicle positions): https://magpie.omnimodal.io/sanfordcra/vehicle-positions

Side question - what's your normal method for capturing an AVL sample and then using it for testing and batch processing? I see some areas in the codebase that seem to support this, and I'd love to be able to replay a set of data to test out different changes quickly.

Thanks,
Nathan

@scrudden
Copy link
Member

scrudden commented May 19, 2018

@nselikoff Can you raise this side question as a new issue and I will detail. As others may be interested and it will be easier for them to find it if it is an issue on its own.

@nselikoff
Copy link
Author

Absolutely, good call @scrudden

@nselikoff
Copy link
Author

Hi @scrudden, following up on this issue. In our branch we had to comment out expectedTravelTimeMsecBackward and just use expectedTravelTimeMsecForward instead of the Math.min calculation in order to avoid problematic temporal matches.

nselikoff added a commit to omnimodal/transitime that referenced this issue Aug 12, 2018
nselikoff added a commit to omnimodal/transitime that referenced this issue Jul 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants