Skip to content

Prune islands with mode-less stop vertices - #5782

Merged
leonardehrenfried merged 4 commits into
opentripplanner:dev-2.xfrom
HBTGmbH:island-pruning-for-stops-without-modes
Apr 12, 2024
Merged

Prune islands with mode-less stop vertices#5782
leonardehrenfried merged 4 commits into
opentripplanner:dev-2.xfrom
HBTGmbH:island-pruning-for-stops-without-modes

Conversation

@bas-hbt

@bas-hbt bas-hbt commented Apr 3, 2024

Copy link
Copy Markdown
Contributor

Summary

For island pruning islands with stops without modes are not considered as only ferries anymore. Currently we have issues with stops that exist in our data that have no service but are linked to small islands.

In the following screenshot you can see the problem
grafik

no modes are not considered as only ferries
@bas-hbt
bas-hbt requested a review from a team as a code owner April 3, 2024 12:25
@codecov

codecov Bot commented Apr 3, 2024

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 66.66667% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 67.87%. Comparing base (c1a9196) to head (8230308).
Report is 30 commits behind head on dev-2.x.

Files Patch % Lines
...aph_builder/module/islandpruning/PruneIslands.java 0.00% 2 Missing and 1 partial ⚠️
...r/graph_builder/module/islandpruning/Subgraph.java 88.88% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##             dev-2.x    #5782      +/-   ##
=============================================
+ Coverage      67.81%   67.87%   +0.06%     
- Complexity     16532    16552      +20     
=============================================
  Files           1906     1908       +2     
  Lines          72275    72337      +62     
  Branches        7443     7441       -2     
=============================================
+ Hits           49015    49102      +87     
+ Misses         20740    20714      -26     
- Partials        2520     2521       +1     

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

@t2gran t2gran added this to the 2.6 (next release) milestone Apr 3, 2024
@bas-hbt bas-hbt changed the title island pruning works for stops without modes fix: island pruning works for stops without modes Apr 4, 2024
@@ -257,7 +257,7 @@ private int processIslands(
TransitStopVertex v = (TransitStopVertex) vIter.next();
Set<TransitMode> modes = v.getModes();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move this code into Subgraph and write documentation about all the cases, in particular the case of the empty modes.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would also make writing a test for it easier, since you only need to test Subgraph not the entire module..

@leonardehrenfried

leonardehrenfried commented Apr 4, 2024

Copy link
Copy Markdown
Member

Also please coordinate with @vesameskanen if the handling of islands with stops with an empty mode need to have a configuration switch. We believe the check for the empty modes was introduced because there are actual islands in Finland that don't have any service in the winter and those should not be pruned.

Another option would be to fix the data in OSM.

@vesameskanen

Copy link
Copy Markdown
Contributor

The reason for considering a stop with no modes as a potential ferry stop was indeed winter time traffic. I think this is quite a specific assumption, and therefore can be changed as proposed in this PR. Removing small islands which have no wintertime connection hardly causes any issues in our routing.

@leonardehrenfried leonardehrenfried changed the title fix: island pruning works for stops without modes Prune islands with mode-less stop vertices Apr 9, 2024

@leonardehrenfried leonardehrenfried left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your latest change is a great improvement.

Comment on lines +139 to +140
for (Iterator<TransitStopVertex> vIter = this.stopIterator(); vIter.hasNext();) {
TransitStopVertex v = vIter.next();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for (Iterator<TransitStopVertex> vIter = this.stopIterator(); vIter.hasNext();) {
TransitStopVertex v = vIter.next();
for (var v : stopsVertexSet) {

Comment on lines +16 to +31
private static RegularStop regularStop1;
private static RegularStop regularStop2;

@BeforeAll
static void setUp() {
regularStop1 =
RegularStop
.of(new FeedScopedId("HH-GTFS", "TEST1"), () -> 0)
.withCoordinate(53.54948, 9.98455)
.build();
regularStop2 =
RegularStop
.of(new FeedScopedId("HH-GTFS", "TEST2"), () -> 0)
.withCoordinate(53.55272, 9.99480)
.build();
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private static RegularStop regularStop1;
private static RegularStop regularStop2;
@BeforeAll
static void setUp() {
regularStop1 =
RegularStop
.of(new FeedScopedId("HH-GTFS", "TEST1"), () -> 0)
.withCoordinate(53.54948, 9.98455)
.build();
regularStop2 =
RegularStop
.of(new FeedScopedId("HH-GTFS", "TEST2"), () -> 0)
.withCoordinate(53.55272, 9.99480)
.build();
}
private static final TEST_MODEL = TransitDataForTest.of();
private static final RegularStop regularStop1 = TEST_MODEL.stop("TEST-1").build();
private static final RegularStop regularStop2 = TEST_MODEL.stop("TEST-2").build();

Comment on lines 102 to 105
for (Iterator<TransitStopVertex> vIter = stopIterator(); vIter.hasNext();) {
Vertex vx = vIter.next();
envelope.expandToInclude(vx.getCoordinate());
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for (Iterator<TransitStopVertex> vIter = stopIterator(); vIter.hasNext();) {
Vertex vx = vIter.next();
envelope.expandToInclude(vx.getCoordinate());
}
for (var vx : stopsVertexSet) {
Vertex vx = vIter.next();
envelope.expandToInclude(vx.getCoordinate());
}

import org.opentripplanner.transit.model.framework.FeedScopedId;
import org.opentripplanner.transit.model.site.RegularStop;

class SubgraphOnlyFerryTest {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great and extensive test! ❤️

/**
* Checks whether the subgraph has only transit-stops for ferries
*
* @return true if only ferries stop at the subgraph and false if other or no other modes are

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @return true if only ferries stop at the subgraph and false if other or no other modes are
* @return true if only ferries stop at the subgraph and false if other or no modes are

Comment on lines +33 to +34
if (vertex instanceof TransitStopVertex) {
stopsVertexSet.add(vertex);
stopsVertexSet.add((TransitStopVertex) vertex);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use a pattern variable here, please.

@leonardehrenfried
leonardehrenfried merged commit 20f0191 into opentripplanner:dev-2.x Apr 12, 2024
t2gran pushed a commit that referenced this pull request Apr 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants