TheTransitClock / transitime

TheTransitClock real-time transit information system
GNU General Public License v3.0
80 stars 30 forks source link

expectedTravelTimeMsec in TemporalMatcher is zero (0) #52

Open nselikoff opened 6 years ago

nselikoff commented 6 years ago

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)?

https://github.com/TheTransitClock/transitime/blob/7bdca846d99dbf0d5b7075dd2b82daeaa63490de/transitclock/src/main/java/org/transitclock/core/TemporalMatcher.java#L398-L412

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 commented 6 years ago

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.

https://github.com/TheTransitClock/transitime/blob/320b165ccd49d7bfb17cc538d7e0e5ae854d9093/transitclock/src/main/java/org/transitclock/core/TravelTimes.java#L400-L403

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 commented 6 years ago

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

nselikoff commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

Absolutely, good call @scrudden

nselikoff commented 6 years ago

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.