bmwcarit / barefoot

Java map matching library for integrating the map into software and services with state-of-the-art online and offline map matching that can be used stand-alone and in the cloud.
Apache License 2.0
665 stars 186 forks source link

Infinite state, unless k/t parameters are specified. #85

Closed fil1o closed 6 years ago

fil1o commented 6 years ago

We use version 0.1.1 in production and it's working fine. However we have issues with the follow up versions. I think we have determined the problem and I'll try to explain.

  1. We don't use the current estimate, because it's often unreliable. We want to be certain about the position so we have implemented our own logic to hold the state until there is only one candidate left in the sequence. This way we know for sure that this is the one and only location of the vehicle.
  2. We don't use k and t parameters, because we clear the state ourselves when there is only one candidate left.

It works with version 0.1.1. In version 0.1.2 however the sequence was modified to hold the current estimate.

Triple<Set, S, C>> sequence

And there is an aditioanal if statement in the remove() function of KState.

    if (sequence.get(index).three() == candidate) {
        return;
    }

It doesn't allow the estimate of the predecessor to be removed. That leads to an infinite state unless there is a time or count limit, which is not our case. In our situation we never get to the point where there is exactly one candidate, so we can't clear the state. Eventually we receive a Stackoverflow exception.

smattheis commented 6 years ago
  1. Do you have a reproducible example for the stack overflow exception?
  2. Is that behavior that you explain some custom implementation from you?
fil1o commented 6 years ago
  1. The exception is in our code. It's caused by the infinite state we keep.
  2. Yes. It's a custom implementation. It's based on your code, but we implement our custom logic.

I'll explain in more detail.

We'd like to be 100% certain about the vehicle position. Unfortunately barefoot doesn't allow that at the moment. Yes you can get the best estimate, but it's often wrong. Here's an example. https://www.darrinward.com/lat-long/?id=5a684e31786528.24392522 I hope what I draw makes sence.

S - state vector C - candidate

S(0)

C1(5%) C2(20%) C3(74%) (most probable, but still wrong) C4(11%)

.....after N points, S(N)...S(0)

S(0)

C2(40%) - the only one left. Now you know for sure this is the correct position.

To get this behaviour we implement our oun logic. We have a wrapper around your KState, that alows us to peek into your private data. We keep the state untill there is only one candidate left in sequence.peekFirst().one(). At this point we mark the point as correctly matched and we cut off the state. It worked perfectly with version 0.1.1.

But with the new version we get kstate-3

C3 is still wrong. There are no transitions from this one to the next position, but it's still here because it was the best estimate at S(0)

So now our custom logic doesn't work, and we are left with an infinite state. I hope what I said makes sence.

smattheis commented 6 years ago

The best solutions is actually to have the essential feature in the mainline. The reason is: (1) Only this way, I can efficiently help on that because everything else is more or less groping in the dark. (2) Only this way, the feature will also be supported in future releases and won't break again. To approach that:

  1. Do you have modified the barefoot code for that feature? (Turned a method from private to public or anything else?)
  2. Have you already filed a pull request for that feature in the past? (I know there are several PRs that haven't been merged in the past, because of https://github.com/bmwcarit/barefoot/issues/81. However, it could be a good opportunity to merge it now if you have a PR and also PR https://github.com/bmwcarit/barefoot/pull/61 since it is affected from that.)

Technically I see two possibilities to address the problem: a) Update the estimate for each vector and for each invocation of the remove method. b) Introduce an explicit mechanism to shorten the k-state once a vector converged. (For now I tend to option b) but a combination of a) and b) could be necessary.)

fil1o commented 6 years ago
  1. No, we haven't modified your source code. We wrapped it in a scala class (it's a scala project) and used reflection to be able to read and manipulate KState.sequence data.
  2. I personally don't feel confident enough to mess around with your code. Writing a wrapper seemed liked the safest option. That way we can still upgrade the library when a new version comes around.

Thinking about a solution, me and my colleagues were thinking about option a) but both solutions will work.

smattheis commented 6 years ago

Okay, i think I've found a very simple solution. However, I have to check that it doesn't break commit https://github.com/bmwcarit/barefoot/commit/cd1b27591da26ea17637cc01fd6d4c427c06e7d9.

smattheis commented 6 years ago

This issue was fixed with commit 25fb150 and is now pushed with version 0.1.4.