Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Island pruning #3426

Merged
merged 27 commits into from
Oct 21, 2021
Merged

Island pruning #3426

merged 27 commits into from
Oct 21, 2021

Conversation

optionsome
Copy link
Member

Summary

This is #2676 ported over to otp2. Additionally, the streetUtils methods have been moved to be under the PruneFloatingIslands class as suggested in the old pr.

Issue

#2675

Unit tests

To be added

Code style

Formatted the PruneFloatingIslands file to follow the code style convention

Documentation

It should be decided if this feature should be configurable as suggested in #2676 (comment) or not.

Changelog

To be added

@optionsome optionsome requested a review from a team as a code owner April 26, 2021 11:15
@optionsome
Copy link
Member Author

There are some things to discuss here:

  • Should this feature be configurable as suggested in Island pruning improvements #2676 (comment) (and split into different modules)?
  • How verbose should the logging be and what should be included in the HTML format report that can be enabled with dataImportReport=true in build-config.json?
  • Should we include unit tests for this feature?

@t2gran t2gran added this to the 2.1 milestone Apr 26, 2021
@t2gran t2gran added the Improvement A functional improvement label Apr 26, 2021
@optionsome
Copy link
Member Author

We discussed this in today's meeting and decided that this new feature should be made configurable so it can be disabled as it increases memory consumption and build time. Also, I'll recheck if the current logging and issue storing works as intended and add some unit tests.

@optionsome optionsome changed the title Island pruning WIP: Island pruning Apr 27, 2021
@optionsome optionsome requested a review from a team as a code owner September 7, 2021 13:44
@vesameskanen
Copy link
Contributor

I hand picked all relevant changes from both mfdz and hsldevcom into one singe commit, to get hopefully right content with decent amount of efforts. Some commit history was lost, sorry for that.

@vesameskanen vesameskanen changed the title WIP: Island pruning Island pruning Sep 14, 2021
changing (especially walk) connectivity of islands with stops can have some undesired
side effects, so log warnings and stats about such changes
@vesameskanen
Copy link
Contributor

A brief summary of changes:

  • Bike, walk and car traverse mode connectivity is processed symmetrically
  • Pruning happens now after stop-street linking, so island classification by stop existence actually works (has been broken before, also in OTP1)
  • Now pruning might remove a small walk network around a linked stop. I tried rerunning stop-street linking after pruning but that seems to crash. Previously, those isolated stops at least sometimes linked to the continuous part of the graph, so this is a minor regression.
  • Old code also included removal of transit links from a pruned island. That code got active after pruning was moved to a later stage, and seems to corrupt the graph (npe error at graph loading). I removed the crashing code - transit links stay alive around isolated stops, which seems harmless.

Current impementation works better than previous versions. Helsinki area data includes one stop whose walk network (2 edges) is removed by pruning. This is most likely an unusual data error which should be fixed.

There are 2 options to tackle the stop isolation problem:

  • Drop island classification by stop existence (this has been the actual process in the past). The risk is that true small networks around stops get removed, for example a ferry stop on an island with only few paths/roads.
  • Find out how to re-link isolated stops after pruning, without crashing the graph builder,

@vesameskanen vesameskanen changed the title Island pruning WIP: Island pruning Sep 15, 2021
Copy link
Member

@t2gran t2gran left a comment

Choose a reason for hiding this comment

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

Look good, only a few things since I last read through this.

vesameskanen and others added 4 commits October 13, 2021 10:05
…ThruIslands.java

Co-authored-by: Thomas Gran <t2gran@gmail.com>
…ThruIslands.java

Co-authored-by: Thomas Gran <t2gran@gmail.com>
…ThruIslands.java

Co-authored-by: Thomas Gran <t2gran@gmail.com>
t2gran
t2gran previously approved these changes Oct 19, 2021
@vesameskanen vesameskanen requested a review from hannesj October 20, 2021 05:31
@t2gran t2gran self-requested a review October 20, 2021 09:47
t2gran
t2gran previously approved these changes Oct 20, 2021
hannesj
hannesj previously approved these changes Oct 20, 2021
issueStore.add(new PrunedIslandStop(v));
}
}
issueStore.add(new GraphIsland(island.getRepresentativeVertex(), island.streetSize()));
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to see traverse modes (original, removed and resulting, as well as if it was a no through traffic island, but those can be added afterwards)

@t2gran t2gran merged commit 915935a into opentripplanner:dev-2.x Oct 21, 2021
@vesameskanen vesameskanen deleted the island-pruning branch October 21, 2021 10:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Improvement A functional improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants