barefoot icon indicating copy to clipboard operation
barefoot copied to clipboard

Type-safe sample propagation through mmatch

Open jongiddy opened this issue 6 years ago • 2 comments

It would be useful to have a MatcherKState generic on the sample class;

public class MatcherKState<S extends MatcherSample> extends KState<MatcherCandidate, MatcherTransition, S>

This would make the code from https://github.com/bmwcarit/barefoot/issues/92#issuecomment-372008797 more type-safe:

class EntendedMatcherSample extends MatcherSample {
    public double speed;
    ExtendedMatcherSample(JSONObject json) {
        super(json);
        speed = json.getDouble("speed");
    }
}

List<ExtendedMatcherSample> samples = new LinkedList<>();

for (int i = 0; i < jsonsamples.length(); ++i) {
  samples.add(new ExtendedMatcherSample(jsonsamples.getJSONObject(i)));
}

MatcherKState<ExtendedMatcherSample> state = matcher.mmatch(samples, minDistance, minInterval);

List<ExtendedMatcherSample> outputSamples = state.samples();
for (ExtendedMatcherSample s: state.samples()) {
    System.out.println("Speed: " + s.speed);
}

jongiddy avatar Mar 10 '18 07:03 jongiddy

That's good. It's a nice and simple enhancement of the API and exploits the underlying generic implementation. However, I'm not sure if we should preserve backwards compatibility:

class MatcherKStateGeneric<S extends MatcherSample> extends KState<MatcherCandidate, MatcherTransition, S> {
    // implementation of old MatcherKState
}

class MatcherKState extends MatcherKStateGeneric<MatcherSample> {
    // constructors
}

and (kind of) overload Matcher#mmatch() as follows (it's a pity that Java doesn't allow overloading on generic types):

public <S extends MatcherSample> MatcherKStateGeneric<S> mmatchg(List<S> samples,
            double minDistance, int minInterval) {
    // ...
}

Optionally, also this one:

public <S extends MatcherSample> void mmatch(MatcherKStateGeneric<S> state, List<S> samples,
            double minDistance, int minInterval) {
    // ...
}

What do you think? I don't want to make the API too complicated.

BTW: I discovered a little design flaw in the implementation of candidate types which have an unnecessary dependency on the sample type as generic argument and which I can resolve with that issue. Only after that it's possible to make k-state generic to MatcherSample without redefining MatcherCandidate.

smattheis avatar Mar 10 '18 11:03 smattheis

I think the implementation looks fine, but I don't like the names MatcherKStateGeneric and mmatchg for the simple reason that the word "generic" is a Java language artifact and has nothing to do with the problem domain.

One possibility is to just use KState and match. The matcher KState isn't much different to a markov.KState apart from some additional output methods. And it's not like anyone is using this code for matching other than map-matching. However, having two methods with such similar names may be error-prone, so maybe matchSamples or similar.

The method signature that includes passing in the state looks like it would allow partial processing of a route, and then adding more samples by calling match on the same state. That sounds like a good idea. And maybe suggests that match could be called addSamples.

However, this would also make it possible to modify the KState while using it, so may need some consideration of thread-safety.

In this case, it would also be a good idea to replace List<S> samples with Iterator<S> samples to provide more flexibility about input generation. (And document that the iterator must provide samples in time order, so the sort is not required).

jongiddy avatar Mar 11 '18 07:03 jongiddy