Optional stop clustering by parent station or geography #2383

barslett · 2016-12-19T10:59:04Z

As discussed in #2364, this PR introduces the possibility to cluster stops based on parent stations, rather than by geographical proximity. This has proved to work well in our test environment here in Oslo.

novalis · 2016-12-30T06:26:07Z

src/main/java/org/opentripplanner/routing/graph/GraphIndex.java

@@ -78,6 +78,9 @@

    /** maximum distance to walk after leaving transit in Analyst */
    public static final int MAX_WALK_METERS = 3500;
+
+    /** Should stop clusters be created by parent stations instead of by proximity?  */
+    public static final boolean CLUSTER_BY_PARENT_STATIONS = false;


This seems sort of funny - it's a final variable with a fixed value, so all of the other code you're adding is dead. Do you mean to make this configurable?

I wanted to make it configurable, yes, and took a shortcut by making this constant. But I see now how I can put it in the router-config, so I will do that instead.

novalis · 2016-12-30T06:26:46Z

src/main/java/org/opentripplanner/routing/graph/GraphIndex.java

-            cluster.computeCenter();
-            stopClusterForId.put(cluster.id, cluster);
-        }
+    	if (!CLUSTER_BY_PARENT_STATIONS) {


It would be better to use a separate class with multiple implementations, if possible.

OK, I will move it to a separate class.

novalis · 2016-12-30T06:27:01Z

src/main/java/org/opentripplanner/routing/graph/GraphIndex.java

+	    	for (Stop s0 : stopForId.values()) {
+	    		String ps = s0.getParentStation();
+	    		parentStopsMap.put(ps, new StopCluster(ps, s0.getName()));
+	    		}


novalis · 2016-12-30T06:27:33Z

src/main/java/org/opentripplanner/routing/graph/GraphIndex.java

+	    		}
+	    	for (Stop s0 : stopForId.values()) {
+	    		String ps = s0.getParentStation();
+	    		if(!parentStopsMap.containsKey(ps)) continue;


Also this if is unnecessary; it will never be true.

OK, will have a look at the logic.

novalis · 2016-12-30T06:30:58Z

src/main/java/org/opentripplanner/routing/graph/GraphIndex.java

+	    	}
+	    	for (Map.Entry<String, StopCluster> cluster : parentStopsMap.entrySet()) {
+	    		cluster.getValue().computeCenter();
+	    		stopClusterForId.put(cluster.getKey(), cluster.getValue());


Doesn't this just mean that stopClusterForId is a duplicate of parentStopsMap?

You may be right. I was staring myself blind on the code...

barslett · 2017-01-02T16:38:37Z

The two new commits do the folllowing:

Adding parameter to router-config
Separating implementations in separate methods
Additional refactoring the method clusterStopsByParentStations

Please review!

barslett · 2017-01-02T20:43:45Z

Most of your comments are about issues already corrected in the second commit (c377195). But I'll fix the cluster name (in our data, parent stations always have the same name as their children, but of course that isn't always the case.

Sorry about text formatting. 1: I am used to .NET formatting, 2: Indentation looks OK in Eclipse...

barslett · 2017-01-02T22:21:17Z

I looked a bit more at using the parent stop's name, and it seems a bit difficult to extract it in this context. As using the child stop's name works perfect for us, and most agencies use similar names for child/parent(??), I suggest that we leave it as it is, and fix it later if anyone else has a need for explicitly using the parent's name?

novalis · 2017-01-03T01:21:06Z

Ok, sorry about the duplicate comments. It's easier to read if you rebase/amend. I'll take a look at the new version soon.

novalis · 2017-01-05T03:02:05Z

src/main/java/org/opentripplanner/routing/graph/GraphIndex.java

+	    }
+    }
+
+	private void clusterByParentStation() {


This indentation is still not right. It's indented by a single tab, while the rest of this file has four spaces. Can you please double-check all of the indentation? Thanks.

novalis · 2017-01-05T03:04:42Z

src/main/java/org/opentripplanner/routing/graph/GraphIndex.java

-            cluster.computeCenter();
-            stopClusterForId.put(cluster.id, cluster);
-        }
+    	if (graph.clusterByParentStation) clusterByParentStation();


newline before clusterByParentStation();

novalis · 2017-01-05T03:05:15Z

src/main/java/org/opentripplanner/routing/graph/GraphIndex.java

+    		StopCluster cluster;
+    		if (stopClusterForId.containsKey(ps)) {
+    			cluster = stopClusterForId.get(ps);		
+    		}


No newline between } and else.

novalis · 2017-01-05T03:05:58Z

src/main/java/org/opentripplanner/routing/graph/GraphIndex.java

+    		}
+    		cluster.children.add(stop);
+    		stopClusterForStop.put(stop, cluster);
+


delete blank line

novalis · 2017-01-05T03:06:08Z

src/main/java/org/opentripplanner/routing/graph/GraphIndex.java

+    	}
+    	for (Map.Entry<String, StopCluster> cluster : stopClusterForId.entrySet()) {
+    		cluster.getValue().computeCenter();
+


also delete this blank line

novalis · 2017-01-05T03:06:13Z

src/main/java/org/opentripplanner/routing/graph/GraphIndex.java

+    		cluster.getValue().computeCenter();
+
+    	}
+	}


novalis · 2017-01-05T03:06:43Z

src/main/java/org/opentripplanner/standalone/Router.java

@@ -151,7 +151,11 @@ public void startup(JsonNode config) {
                }
            }
        }
-
+


don't replace a blank line with a line containing only trailing whitespace

(this one seems to still be there)

novalis · 2017-01-05T03:09:18Z

src/main/java/org/opentripplanner/standalone/Router.java

+
+        JsonNode clusterByParentStation = config.get("clusterByParentStation");
+        if (clusterByParentStation != null && clusterByParentStation.isBoolean()) 
+        	graph.clusterByParentStation = clusterByParentStation.asBoolean();


... and if it's not a boolean, throw an exception or something.

But actually, it should be a string called something like "clusterMode", with values "parentStation" and "geographic" -- maybe later someone will add something that clusters by name or something.

barslett · 2017-01-05T17:11:16Z

What a hassle with the formatting ;-). At least, I have now installed the preferred text formatter for Eclipse, so I hope these problems will be reduced for the future.

I also added some logging for cases when the parameter is falsely set to a string value. Of course the whole logic could be rewritten to facilitate additional clustering methods, but I would prefer not having to do that at the moment.

novalis · 2017-01-06T06:22:27Z

I think it would not be hard to do the parameter as a string, and I think it would be much cleaner.

barslett · 2017-01-06T08:10:46Z

OK. Figured it out in the shower. Will update today.

novalis · 2017-01-08T00:18:02Z

Thanks for all your hard work on this.

I've gone ahead and merged it even though Travis is failing, since when I test locally, the tests pass.

barslett · 2017-01-08T00:19:20Z

Thanks, I also wonder what happened, as the tests pass locally here as well.

…tripplanner#2383)" This reverts commit f142150

Optional stop clustering by parent station or geography

bcab05f

novalis requested changes Dec 30, 2016

View reviewed changes

barslett added 2 commits January 2, 2017 00:29

Introduced router-config parameter for stop clustering

5d230cc

Refactoring

c377195

Formatting if-clauses

2a3e208

novalis requested changes Jan 5, 2017

View reviewed changes

Corrected formatting and added logging for invalid parameter value

31b7bc4

barslett added 2 commits January 7, 2017 23:45

Changed parameter to stopClusterMode (String)

c7598cb

Initializing default value for stopClusterMode

30e574a

novalis merged commit f142150 into opentripplanner:master Jan 8, 2017

barslett mentioned this pull request Jan 16, 2017

Fixed NullPointerException if clustering before graph creation #2394

Merged

barslett mentioned this pull request Aug 11, 2017

Clustering by parent station fix #2447

Merged

t2gran added a commit to entur/OpenTripPlanner that referenced this pull request Apr 12, 2018

Revert "Optional stop clustering by parent station or geography (open…

c9836c6

…tripplanner#2383)" This reverts commit f142150

t2gran added a commit to entur/OpenTripPlanner that referenced this pull request Apr 12, 2018

Revert "Optional stop clustering by parent station or geography (open…

74db73d

…tripplanner#2383)" This reverts commit f142150

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optional stop clustering by parent station or geography #2383

Optional stop clustering by parent station or geography #2383

barslett commented Dec 19, 2016

novalis Dec 30, 2016

barslett Dec 31, 2016

novalis Dec 30, 2016

barslett Dec 31, 2016

novalis Dec 30, 2016

barslett Dec 31, 2016

novalis Dec 30, 2016

novalis Dec 30, 2016

barslett Dec 31, 2016

novalis Dec 30, 2016

barslett Dec 31, 2016

barslett commented Jan 2, 2017

barslett commented Jan 2, 2017

barslett commented Jan 2, 2017

novalis commented Jan 3, 2017

novalis Jan 5, 2017

novalis Jan 5, 2017

novalis Jan 5, 2017

novalis Jan 5, 2017

novalis Jan 5, 2017

novalis Jan 5, 2017

novalis Jan 5, 2017

novalis Jan 6, 2017

novalis Jan 5, 2017

barslett commented Jan 5, 2017 •

edited

Loading

novalis commented Jan 6, 2017

barslett commented Jan 6, 2017

novalis commented Jan 8, 2017

barslett commented Jan 8, 2017

@@ @@ -151,7 +151,11 @@ public void startup(JsonNode config) { @@
                               }
                           }
                       }

Optional stop clustering by parent station or geography #2383

Optional stop clustering by parent station or geography #2383

Conversation

barslett commented Dec 19, 2016

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

barslett commented Jan 2, 2017

barslett commented Jan 2, 2017

barslett commented Jan 2, 2017

novalis commented Jan 3, 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

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

barslett commented Jan 5, 2017 • edited Loading

novalis commented Jan 6, 2017

barslett commented Jan 6, 2017

novalis commented Jan 8, 2017

barslett commented Jan 8, 2017

barslett commented Jan 5, 2017 •

edited

Loading