WIP: handling viterbi breaks as multiple sequences #87

kodonnell · 2016-12-12T06:22:45Z

Objective

If the viterbi sequence breaks (e.g. no candidates found for a given GPX entry, or no transitions between candidates) then instead of throwing an exception, split into a new sequence. As far as I'm aware there's currently no easy way for the user to do this manually, let alone easily.

Questions

is this sensible behaviour for the user?
should we return a single MapMatch result (with a single time/distance) - as it implemented in this first commit - or multiple MapMatch results (each within their own time/distance). I prefer the latter (with a reason why each break occurred).
specific comments below
anything else?

Cc @karussell @stefanholder

kodonnell · 2016-12-12T06:23:47Z

matching-core/src/main/java/com/graphhopper/matching/MapMatching.java

@@ -166,6 +166,8 @@ public MatchResult doWork(List<GPXEntry> gpxList) {
        // Compute all candidates first.
        // TODO: Generate candidates on-the-fly within computeViterbiSequence() if this does not
        // degrade performance.
+        // @kodonnell: it's not currently possible as that'd mean calling queryGraph.lookup


@karussell / @stefanholder - happy for me to remove the above TODO?

@stefanholder I probably do not understand this yet deep enough, but why do you think this would mean calling queryGraph.lookup multiple times?

Furthermore: creating a QueryGraph is cheap so we could call it then multiple times but the resulting QueryResults are independent.

Not sure if you meant me - but I think it requires calling queryGraph.lookup multiple times because we're wanting to get the candidates at each step. E.g. first step: get candidates, lookup, do viterbi next step; second step: get candidates, lookup, do viterbi next step, ...

If we create multiple queryGraphs, then we'll get duplicate virtual edge IDs, etc., which might be a problem.

I think that looking up all candidates at once might be a problem for map matching very long GPS traces, e.g. from Munich to Hamburg. But maybe it's better to create an issue for this than having this TODO in the code.

Good point @stefanholder - hopefully #90 will suffice.

kodonnell · 2016-12-12T06:42:32Z

matching-core/src/main/java/com/graphhopper/matching/MapMatching.java

+                // recent step does not get added i.e. 'computeMostLikelySequence' returns the most
+                // likely sequence without this (breaking) step added. Hence we can use it to start
+                // the next one:
+                // TODO: check the above is true


@stefanholder - I think this is true, but can you confirm? I guess I could check for it to some degree ...

Yes, your comment is true.

kodonnell · 2016-12-12T06:45:53Z

matching-core/src/main/java/com/graphhopper/matching/MapMatching.java

                final Path path = algo.calcPath(from.getQueryResult().getClosestNode(),
                        to.getQueryResult().getClosestNode());
                if (path.isFound()) {
                    timeStep.addRoadPath(from, to, path);
                    final double transitionLogProbability = probabilities
                            .transitionLogProbability(path.getDistance(), linearDistance, timeDiff);
                    timeStep.addTransitionLogProbability(from, to, transitionLogProbability);
+                } else {
+                    // TODO: can we remove maxVisitedNodes completely and just set to infinity?


@karussell - what is the purpose of having maxVisitedNodes in map-matching? In theory we can more easily prevent large graph traversals by limiting linear distance ... and then the user doesn't need to worry about configuring this option (and we have a 'physical' reason for throwing an error or finding no candidate - there were no transitions found within a given radius).

Doing an unlimited search is not good, especially not if you want roughly predicted maximum processing times. Linear distance is easier but graphs are often disconnected due to rivers etc where a distance limitation still can result in very costly traversals. Maybe we should use some good defaults and split the result instead of throwing an exception.

Fair point. However, if I'm a user, what do I do if I get the above exception? If I want a map match, there's nothing I can do except increase maxVisitedNodes. So we might as well set it large by default anyway. If the user wants to handle the exceptions themselves (I can't really see why) then they can override the maxVisitedNodes to something smaller.

As an aside - the default GH behaviour is presumably to have no limit on maxVisitedNodes? Same argument applies there, I think.

If I want a map match, there's nothing I can do except increase maxVisitedNodes

If you have full control of the library&server then just use a maximum value, but defaults should be safe so that not a single request could occupy one CPU for dozens of seconds.

As an aside - the default GH behaviour is presumably to have no limit on maxVisitedNodes?

No. We have a max nodes limits too for non-CH routes. But the limits are much bigger because the number of points are typically a lot less.

OK. I'll leave it throwing an error for now. In future, we could maybe think of adding e.g. matchResult.isFound() similar to path.isFound().

kodonnell · 2016-12-12T06:48:16Z

matching-core/src/main/java/com/graphhopper/matching/MapMatching.java

+                    }
+                    // TODO: can we somewhere record that this route failed? Currently all viterbi
+                    // knows is that there's no transition possible, but not why. This is useful
+                    // for e.g. explaining why a sequence broke.


If we split into multiple MatchResult (one per each sequence) then maybe we could add a 'sequenceStartReason' property? For the first sequence it will be 'first sequence' and for the rest it might be 'no candidates found' or 'no transitions found' etc. @karussell / @stefanholder ?

Sounds good to me, but I'm not sure if this is helpful for the user?

Me neither. My thinking was that breaking into sequences is better than throwing an exception. However, I then thought that the user may want to know why the sequence broke. And it might be useful for diagnostics etc.

Yes, I agree. Having separated sequences is the way to go IMO. And if user requests specific reasons we can also fine tune this later on.

Good point - will leave as a TODO for now.

kodonnell · 2016-12-12T06:48:58Z

matching-core/src/main/java/com/graphhopper/matching/MapMatching.java

+    private MatchResult computeMatchedEdges(
+            List<List<SequenceState<GPXExtension, GPXEntry, Path>>> sequences,
+            Map<String, EdgeIteratorState> virtualEdgesMap) {
+        // TODO: remove gpx extensions and just add time at start/end of edge.


@karussell - can I do this? I'm not sure why else we save all the gpxExtensions to the given edge ..

Could you show what you want to do :) ? Currently not sure what you mean

Sorry - I want to remove all GPX extensions from edge matches (and hence match result), and add only the to/from time to the edge (which will be inferred from the GPX points and route times, etc.).

The idea is that in the future it should (correctly) associate all original GPXEntry's to one edge. See e.g. https://discuss.graphhopper.com/t/map-matchresult-to-gpxentry/977

Oh, that should (hopefully) be easy - I need to find the matching GPX entry to infer the start/end time anyway. Separate PR?

Separate PR?

Would be welcome :) ! But you would need to use some additional data and so we keep the GPXExtensions in the EdgeMatch?

Not sure - will discuss in the (hopefully soon) PR.

stefanholder · 2016-12-13T09:46:19Z

is this sensible behaviour for the user?

Yes, this makes sense.

should we return a single MapMatch result (with a single time/distance) - as it implemented in this first commit - or multiple MapMatch results (each within their own time/distance). I prefer the latter (with a reason why each break occurred).

I think that MatchResult should contain the matched GPS position for each original GPS position, as discussed above. Then each such entry could also contain a field indicating if/why an HMM break occurred. I think this is easier to handle by the user than multiple map match results. The user might not even be interested in HMM breaks and getting a single MapMatch result is then easier to handle.

kodonnell · 2016-12-13T21:15:45Z

I think that MatchResult should contain the matched GPS position for each original GPS position, as discussed above. Then each such entry could also contain a field indicating if/why an HMM break occurred.

Ah, that's a good idea. So, to clarify, we basically want one MapMatch result which includes the final route, as well as all the original GPX entries. Each entry can be inspected to know:

the edge (of the route) that it is on (if any)
some sort of status flag - 'start of sequence', 'single candidate', 'not used in map matching as too close to previous candidate', etc.

I'd suggest we also add a more general status flag to the MapMatch result e.g. 'contains sequence breaks', so users can easily check for this. And maybe we can warn the user too. (Otherwise if it's used blindly, we may end up with lots of issues submitted asking why there is a 'break' in the matched route!)

Anyway, with this, we should be able to describe all situations. However, as you say, most users will probably only be interested in the final route.

@stefanholder - does that sound good to you?

stefanholder · 2016-12-14T09:09:47Z

@stefanholder - does that sound good to you?

Yes, sounds good.

I would put it like this: MatchResult should contains the final route as well as a list of match entries.

Each match entry contains:

the original GPX entry
the map matched lat/lon
the edge (of the route) that it is on (if any) and the offset from the start of the edge (or alternatively the fraction as offset divided by edge length)
HMM break status: 'no break', 'no candidates', 'no transitions to this entry'
'used for map matching' or 'not used in map matching as too close to previous candidate'

kodonnell · 2016-12-14T18:30:00Z

I would put it like this:

Great, sounds good. I'll look over your other PR first, and then have a play with all of the comments above.

kodonnell · 2017-01-13T02:36:12Z

You can ignore the code for now (it's still unfinished), but I'd like some feedback on progress thus far. Specifically:

I realised I didn't want to directly add in the GPX entries not used for map matching. Why? Because if the user e.g. loops through all of the GPX entires (including those not used for map-matching) they resulting sequence may look a lot more erratic and behave unexpectedly. (E.g. if I go from A -> B, when I add in the ignored points, it might be 'go toward B, then back toward A, then toward B ...'.) I think it makes more sense to think of clustering - i.e. points that are close to each other are the 'same'. For this reason, I do not snap the GPX entries (that aren't used) to the final route - but merely associate them with their 'cluster'. This means we avoid the erratic nature, but also that we can find where any of the original GPX entries is in the final route (up to the GPS error).
- an alternative option is to leave it as is ('ignored' GPX entries are ignored from the result), but allow the user to change the filtering to prevent e.g. any filtering at all. This is simpler, but will slow down the matching.
I think the MatchSequence does most of what @stefanholder suggested above (though maybe not tidily yet). I provide a MatchResult.originalGPXMapping which allows the user to map (by index) a given original GPX entry to the matched one - and this contains information like the matched edge, how far along, the snapped point, the to/from time, etc. Alternatively one can traverse the matches directly.
- a tidier (API and code) approach would be to add properties directly to the original GPXEntry. It means not using the GH GPXEntry (just extending it) but this probably isn't a big deal.
when we have sequence breaks, should I fill the break with an 'empty' sequence break, to keep the times contiguous? I.e. that way a user should always find a sequence at a given time.

The main changes remaining are

refactoring to get it nice and clean (I'll wait a few days and come back with a fresh view)
make sure the to/from times are contiguous.
trivial-ish refactoring of tests, etc.
sequence-specific tests.
refactoring the web GUI to handle sequences.
update all doco

stefanholder · 2017-01-13T09:03:41Z

an alternative option is to leave it as is ('ignored' GPX entries are ignored from the result), but allow the user to change the filtering to prevent e.g. any filtering at all. This is simpler, but will slow down the matching.

I would prefer this alternative because it's simpler and I think the user should have the option to disable the filtering.

a tidier (API and code) approach would be to add properties directly to the original GPXEntry

I think it's better to have a separate class (GPXMapping) for the matched GPX entry.

when we have sequence breaks, should I fill the break with an 'empty' sequence break, to keep the times contiguous?

If there are no candidates for a GPX entry then GPXMapping.matchEntry should be null (not sure if this is what you mean).

kodonnell · 2017-01-13T18:30:20Z

Thanks @stefanholder - I will follow your first two preferences.

If there are no candidates for a GPX entry then GPXMapping.matchEntry should be null (not sure if this is what you mean).

More if e.g. we have a sequence A->C then an HMM break so start a new sequence D->F. In this case, there's no (explicit) information about what happened between C and D - we could leave it to the user to interpret, or I could add an 'unknown'/'empty' sequence C->D which really only contains the information about the start/end points (including time) but e.g. the route between them will be 'unknown'. Does that make sense?

kodonnell · 2017-01-15T01:57:54Z

To keep PR from getting even bigger, I created #92 for allowing customisable filtering. Otherwise, this PR is stabilising somewhat. I've renamed a lot of the entities - the names were misleading, and were confusing me (let alone new developers). E.g. GPXExtension -> Candidate, TimeStep -> ViterbiMatchEntry. I've updated the list above with some more tasks remaining too.

should I fill the break with an 'empty' sequence break

For now I'll leave this out - it's non-essential, and isn't too hard to add later if it's desired.

stefanholder · 2017-01-16T15:05:37Z

More if e.g. we have a sequence A->C then an HMM break so start a new sequence D->F.

If an HMM break happens because there is no transition between C and D then we can set the HMM break status for the GPXMapping of D to 'no transitions to this entry', which is quite simple to do.

stefanholder · 2017-03-01T17:35:22Z

I've not checked it closely yet, maybe @stefanholder did?

No, I didn't look at it closely yet either.

stefanholder · 2017-03-14T10:55:01Z

matching-core/src/main/java/com/graphhopper/matching/MatchEdge.java

+    public final long toTime;
+
+    public MatchEdge(EdgeIteratorState edge, long fromTime, long toTime) {
+        assert edge != null : "edge should not be null";


If a precondition of a public method is violated, an exception should rather be thrown (NullPointerException in this case).

Thanks - may be a few more of those. Will look now.

stefanholder · 2017-03-14T11:01:32Z

matching-core/src/main/java/com/graphhopper/matching/MatchSequence.java

+     * Time (inclusive, in milliseconds) when first began on this sequence. -1 if not set.
+     * TODO: is in inclusive?
+     */
+    private long fromTime = -1;


I would prefer using Long and setting this to null.

Made it final instead of long - it should never be null or -1 now.

OK, this is even better.

stefanholder · 2017-03-14T11:01:41Z

matching-core/src/main/java/com/graphhopper/matching/MatchSequence.java

+     * Time (inclusive, in milliseconds) when last was on this sequence. -1 if not set.
+     * TODO: is in inclusive?
+     */
+    private long toTime = -1;


stefanholder · 2017-03-14T11:11:42Z

matching-core/src/main/java/com/graphhopper/matching/MatchEntry.java

+        locked = true;
+    }
+
+    protected void saveMatchingState(int sequenceIdx, int sequenceMatchEdgeIdx,


The method is called externally (e.g. here) and I don't want it to be available to users - I could be missing something, but isn't this the purpose of 'protected'?

In this case you should use package-private (no modifier) instead of protected. Protected should only be used if you plan to access the method in derived classes.

Sorry about that, and thanks for pointing it out.

stefanholder · 2017-03-14T11:12:06Z

matching-core/src/main/java/com/graphhopper/matching/MatchEntry.java

+        this.gpxEntry = gpxEntry;
+    }
+
+    protected void markAsNotUsedForMatching() {


Why is this protected, do you plan to inherit from this class?

stefanholder · 2017-03-14T12:36:07Z

matching-core/src/main/java/com/graphhopper/matching/util/ViterbiMatchEntry.java

@@ -67,7 +67,7 @@ public int compare(QueryResult o1, QueryResult o2) {
        }
    };

-    public TimeStep(MatchEntry matchEntry) {
+    public ViterbiMatchEntry(MatchEntry matchEntry) {


ViterbiMatchEntry sounds like this class represents the output of the Viterbi matching but it is actually the input of the Viterbi algorithm. How about HmmTimeStep?

I can't remember now why I made that change = ) Anyway, I agree with you.

stefanholder · 2017-03-14T13:00:21Z

matching-web/src/main/java/com/graphhopper/matching/http/MatchServlet.java

+//                PathMerger pathMerger = new PathMerger().
+//                        setDouglasPeucker(peucker).
+//                        setSimplifyResponse(wayPointMaxDistance > 0);
+//                pathMerger.doWork(matchGHRsp, Collections.singletonList(path), tr);


Why was this commented out? If the code is really not used anymore it should be deleted.

As I didn't know how to get it working - I'll update that when I get the web stuff working.

stefanholder · 2017-03-14T13:02:01Z

matching-web/src/test/java/com/graphhopper/matching/http/MatchResultToJsonTest.java

-        Assert.assertEquals("wpts[1].x should exists", -38.9999, link.getJSONArray("wpts").getJSONObject(1).get("x"));
-
-    }
+//    protected List<MatchEdge> getEdgeMatch() {


What's the reason you gave up on this test?

stefanholder · 2017-03-14T13:06:12Z

matching-core/src/main/java/com/graphhopper/matching/MatchEdge.java

- *
+ * A MatchEdge is an edge on a MatchSequence - i.e. one of the edges making up the final
+ * map-matched route. This includes the time at which the edge was travelled.
+ * 
 * @author kodonnell
 */
 public class MatchEdge {


How about MatchedEdge instead of MatchEdge?

I think I was trying to be consistent with (the original) MatchResult as opposed to MatchedResult - happy to change all if you wish?

I don't think that consistency is needed here, so I would be happy with MatchedEdge (the matched edge) and MatchResult (the result is not matched, so I think this is fine).

stefanholder · 2017-03-14T13:13:21Z

README.md

@@ -1,125 +0,0 @@
-## Map Matching based on GraphHopper


Seems that README.md was accidentally removed.

Not sure what I did there ...

stefanholder · 2017-03-14T13:16:21Z

matching-core/src/main/java/com/graphhopper/matching/LocationIndexMatch.java

- *
- * @author Peter Karich
- */
-public class LocationIndexMatch extends LocationIndexTree {


I would prefer to have this logic in a separate class since this is a feature that could also be used for other purposes besides map matching. I think this class should actually be moved to the Graphhopper core.

I think it makes sense for the method findCandidateLocations to be part of HmmTimeStep as it's currently laid out - it felt cleaner to me (mainly because the code is only used for this one purpose). However, if Graphhopper had this index logic implemented, then yes, it'd be nicer, and the findCandidateLocations method would be much simpler. I'm not too worried though - if you want me to change it, I'm happy to.

@karussell, do you think that LocationIndexMatch should be moved to the Graphhopper core?

yes
(probably either with different name or separate class method)

I'll look into it

stefanholder · 2017-03-14T13:19:08Z

matching-core/src/main/java/com/graphhopper/matching/MatchSequence.java

+    /**
+     * The matched sequence, as returned from viterbi.computeMostLikelySequence().
+     */
+    public final List<SequenceState<Candidate, MatchEntry, Path>> matchedSequence;


I think the map matching result should not contain SequenceState objects since these include internal data that is not relevant for the user. Hence, the relevant information should be extracted from SequenceState into separate result objects.

I could just make it private? The user information (edges, original points in sequence, etc.) can be obtained by the user without it.

Sure, this is also fine.

Had to be package-private as it's used in MapMatching.java for debugging.

OK, no problem.

stefanholder

I could not review all changes in depth because there were just so many changes by refactorings.

Because I was reviewing and annotating this PR commit by commit, some of my recent comments are shown as outdated. I think they are still valid.

What about the remaining non-checked items?

sequence-specific tests.
refactoring the web GUI to handle sequences.
update all doco

kodonnell · 2017-03-20T06:56:45Z

What about the remaining non-checked items?

They're yet to be completed. Are you happy with the sequence-specific stuff of this PR (logic and API)? If so, I'll work on those additional items.

stefanholder · 2017-03-20T14:51:48Z

Are you happy with the sequence-specific stuff of this PR (logic and API)?

I'm happy with it. But since the API has changed a lot, I think it's important that @karussell also has a look.

kodonnell · 2017-03-20T17:48:06Z

Great. I'll review your other comments, and work on the remaining items. When that's done - yes, I think it'd be good to have @karussell review.

kodonnell · 2017-03-28T02:30:10Z

Wow, getting the GUI to handle multiple paths was much more painful than I thought. Anyway:

That's using this gpx. I also had to tweak locationIndexTree.findNClosest to make it not always return a result (within tiles but outside radius) - otherwise I couldn't break it into sequences. (See here.)

Yes!!

As mentioned, the GUI thus far is hacky (and not fully-functioning) - I almost think it'd be better to rewrite from scratch than to continue hacking around it. However, then I'd risk making breaking changes (i.e. features I don't understand) ... so I'm not sure.

The unit tests are probably more important - I discovered some bugs even just testing this.

We're starting to build our house, so this is becoming less of a priority for me. I might still do it, but my I shouldn't make promises I can't keep ...

karussell · 2017-03-28T07:27:15Z

Cool, thanks a lot :) !

We're starting to build our house, so this is becoming less of a priority for me

Sure :) !

INRIX-Trang-Nguyen · 2018-07-18T08:06:40Z

Did these changes make into the latest code base? We're finding very few matches when inspecting GPXExensions of match result. We have hundreds of input GPX entries but GPXExtension contains in many cases only a single entry.

kodonnell · 2018-07-18T09:13:26Z

@INRIX-Trang-Nguyen not that I know of. I'm closing this as I have no intention of actively working on it.

initial work for handling viterbi breaks as multiple sequences

2c1a010

kodonnell commented Dec 12, 2016

View reviewed changes

handle case where viterbi breaks immediately after initialization

d979ffd

kodonnell commented Dec 12, 2016

View reviewed changes

This was referenced Dec 13, 2016

Penalize inner-link U-turns (builds on #83) #88

Merged

compute candidates on the fly if possible? #90

Open

kodonnell mentioned this pull request Dec 30, 2016

Association of all provided GPXEntry to the corresponding EdgeMatch #64

Open

kodonnell added 2 commits December 31, 2016 10:21

merge U-turn work

6b856f1

refactoring sequences

ade4037

kodonnell added 7 commits January 15, 2017 07:48

use MatchEntry internally instead of GPXEntry

4b24cbc

debug and fix tests

5832623

rename timestep -> viterbimatchentry

b6c3af4

fix other tests

c0e5574

tidying calcpath and gpxfile/main

2d184a2

web stuff ...

791e53c

giving up on that test ...

eed78bf

kodonnell mentioned this pull request Jan 15, 2017

give user control of filtering before map-matching #92

Open

woops, don't need that anymore ...

ac105e7

stefanholder reviewed Mar 14, 2017

View reviewed changes

stefanholder suggested changes Mar 14, 2017

View reviewed changes

kodonnell added 3 commits March 20, 2017 19:22

some changes as per @stefanholder

4a7420a

bringing back the missing readme

13707e1

a few more tidyups

efb7b57

more tidy-ups

956e7d0

This was referenced Mar 20, 2017

Migrate map-matching LocationIndexMatch to graphhopper graphhopper/graphhopper#993

Closed

moved LocationIndexMatch.findNClosest from map-matching to LocationIndexTree graphhopper/graphhopper#994

Merged

kodonnell added 2 commits March 21, 2017 09:02

utilise LocationIndexTree.findWithinRadius

1dd88e8

ugly hacky gui ...

56df0ab

kodonnell closed this Jul 18, 2018

WIP: handling viterbi breaks as multiple sequences #87

WIP: handling viterbi breaks as multiple sequences #87

Conversation

kodonnell commented Dec 12, 2016

Objective

Questions

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanholder commented Dec 13, 2016

kodonnell commented Dec 13, 2016

stefanholder commented Dec 14, 2016

kodonnell commented Dec 14, 2016

kodonnell commented Jan 13, 2017 • edited Loading

stefanholder commented Jan 13, 2017

kodonnell commented Jan 13, 2017

kodonnell commented Jan 15, 2017

stefanholder commented Jan 16, 2017

stefanholder commented Mar 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanholder Mar 14, 2017 • edited Loading

Choose a reason for hiding this comment

stefanholder Mar 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanholder Mar 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanholder Mar 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanholder left a comment

Choose a reason for hiding this comment

kodonnell commented Jan 13, 2017 •

edited

Loading

stefanholder Mar 14, 2017 •

edited

Loading

stefanholder Mar 14, 2017 •

edited

Loading

stefanholder Mar 14, 2017 •

edited

Loading

stefanholder Mar 14, 2017 •

edited

Loading