Consider escalator edges in island pruning by nurmAV · Pull Request #5591 · opentripplanner/OpenTripPlanner

nurmAV · 2023-12-21T11:11:32Z

Summary

This PR adds consideration of escalator edges in island pruning. Previously, street edges that were only connected to escalator edges were pruned out.

Issue

When escalator edges were added in a previous PR, they were not considered in island pruning, leading to some their neighbouring street edges being pruned out.

Unit tests

The change was tested manually by checking the street network in OTP debug UI.

codecov · 2023-12-21T11:17:56Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c766bde) 67.46% compared to head (bb19842) 67.46%.
Report is 41 commits behind head on dev-2.x.

Additional details and impacted files

@@              Coverage Diff              @@
##             dev-2.x    #5591      +/-   ##
=============================================
- Coverage      67.46%   67.46%   -0.01%     
+ Complexity     16194    16191       -3     
=============================================
  Files           1858     1862       +4     
  Lines          71130    71184      +54     
  Branches        7408     7401       -7     
=============================================
+ Hits           47986    48021      +35     
- Misses         20669    20673       +4     
- Partials        2475     2490      +15

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

leonardehrenfried · 2023-12-21T11:20:59Z

I'm afraid I'm going to have to ask you to write a test for this. You could:

construct a graph manually (preferred but a bit complicated. i'm happy to help)
extract a tiny piece of OSM data like this and write a test with it. @vesameskanen can help you with this.

leonardehrenfried · 2023-12-21T18:04:13Z

+
+  @Test
+  public void streetEdgesBetweenEscalatorEdgesRetained() {
+    Assertions.assertTrue(


Please use a static import for that.

leonardehrenfried · 2023-12-21T18:05:42Z

+  private static Graph buildOsmGraph(File osmFile) {
+    try {
+      var deduplicator = new Deduplicator();
+      var graph = new Graph(deduplicator);
+      var transitModel = new TransitModel(new StopModel(), deduplicator);
+      // Add street data from OSM
+      OsmProvider osmProvider = new OsmProvider(osmFile, true);
+      OsmModule osmModule = OsmModule.of(osmProvider, graph).withEdgeNamer(new TestNamer()).build();
+
+      osmModule.buildGraph();
+
+      transitModel.index();
+      graph.index(transitModel.getStopModel());
+
+      // Prune floating islands and set noThru where necessary
+      PruneIslands pruneIslands = new PruneIslands(
+        graph,
+        transitModel,
+        DataImportIssueStore.NOOP,
+        null
+      );
+      pruneIslands.setPruningThresholdIslandWithoutStops(40);
+      pruneIslands.setPruningThresholdIslandWithStops(5);
+      pruneIslands.setAdaptivePruningFactor(1);
+      pruneIslands.buildGraph();
+
+      return graph;
+    } catch (Exception e) {
+      throw new RuntimeException(e);
+    }
+  }


This method is a direct copy of PruneNoIslandsTest. Please extract a reusable method.

Just to clarify, where should the extracted method be placed? Since it would be used in some other test files as well, it would seem a bit weird to import it as a static from this class as that would imply this special test has some general functionality that other tests need

Make a package-private class and put the static method there.

leonardehrenfried · 2023-12-21T18:06:17Z

+
+  @BeforeAll
+  static void setup() {
+    graph =


Since it's only one test, you don't need the global setup method. Just build the graph in the test itself.

vesameskanen · 2024-01-02T17:02:52Z

Please use the new graph build method for adaptive pruning test as well. Just pass pruning params to graph builder.

vesameskanen · 2024-01-02T17:00:04Z

+        .map(streetEdge -> streetEdge.getName().toString())
+        .collect(Collectors.toSet())
+        .containsAll(Set.of("490072445"))
+    );


contains("490072445")

don't use a set of single member

leonardehrenfried · 2024-01-04T13:54:52Z

            e instanceof StreetEdge ||
            e instanceof ElevatorEdge ||
            e instanceof FreeEdge ||
-            e instanceof StreetTransitEntityLink
+            e instanceof StreetTransitEntityLink ||
+            e instanceof EscalatorEdge


@vesameskanen Would it be a good idea to convert these instanceOf checks into a method on Edge instead?

(I'm not going to expect @nurmAV to do it in this PR.)

I am quite sure that there is a need for refactoring. Use of instanceof indicates that something has gone wrong in first place. Adding a proper method sounds like a correct fix.

However, I am not 100% sure if there is a need for considering only certain type of edges at all. Maybe pruning works fine in 99.99% of cases even if all edges are considered.

I will test this - it is easy to check the pruning stats and see if selective edge traversal is necessary.

I found out that helsinki area pruning works exactly the same way, when all edges are considered. I suggest removing edge class test.

I am sorry that this comment comes this late. Somehow the new test now appears redundant. Test refactoring is useful, though.

Good investigation. I think this code may have been from a time when we had transit edges (TransitHopEdge) in the graph and you would probably never want to prune them.

leonardehrenfried · 2024-01-04T18:54:03Z

If you apply these cosmetic changes to the PR, I'm happy to approve: leonardehrenfried@a8086b1

vesameskanen

Thank you for the cleanup, Leonard!

Consider escalator edges in island pruning

b7ea0b2

nurmAV requested a review from a team as a code owner December 21, 2023 11:11

Add test for island pruning between escalators

82e843b

leonardehrenfried requested review from leonardehrenfried and vesameskanen December 21, 2023 15:10

leonardehrenfried reviewed Dec 21, 2023

View reviewed changes

leonardehrenfried added the !Bug Apply to issues describing a bug and PRs witch fixes it. label Dec 22, 2023

viljaminurminen-cgi added 2 commits January 2, 2024 12:42

Extract static method for OSM graph building

5c8e847

Make OSM graph building method package-private

9138ba0

vesameskanen reviewed Jan 2, 2024

View reviewed changes

viljaminurminen-cgi added 2 commits January 4, 2024 13:24

Generalize graph building method

e132e88

Clean up debugging comments

62865b0

leonardehrenfried reviewed Jan 4, 2024

View reviewed changes

t2gran added this to the 2.5 (next release) milestone Jan 4, 2024

Remove unnecessary edge selection

cae4145

vesameskanen self-requested a review January 4, 2024 18:18

vesameskanen previously approved these changes Jan 4, 2024

View reviewed changes

Cosmetic changes to island pruning tests

a8086b1

vesameskanen dismissed their stale review via a8086b1 January 5, 2024 06:30

vesameskanen self-requested a review January 5, 2024 06:31

vesameskanen previously approved these changes Jan 5, 2024

View reviewed changes

Fix formatting

bb19842

vesameskanen dismissed their stale review via bb19842 January 5, 2024 06:46

vesameskanen approved these changes Jan 5, 2024

View reviewed changes

leonardehrenfried approved these changes Jan 5, 2024

View reviewed changes

vesameskanen merged commit d71c768 into opentripplanner:dev-2.x Jan 5, 2024

vesameskanen deleted the DT-6123 branch January 5, 2024 07:02

t2gran pushed a commit that referenced this pull request Jan 5, 2024

Add changelog entry for #5591 [ci skip]

7342f01

Conversation

nurmAV commented Dec 21, 2023 • edited by leonardehrenfried Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Issue

Unit tests

Uh oh!

codecov Bot commented Dec 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

leonardehrenfried commented Dec 21, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vesameskanen commented Jan 2, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vesameskanen Jan 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leonardehrenfried commented Jan 4, 2024

Uh oh!

vesameskanen left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nurmAV commented Dec 21, 2023 •

edited by leonardehrenfried

Loading

codecov Bot commented Dec 21, 2023 •

edited

Loading

vesameskanen Jan 4, 2024 •

edited

Loading