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

Reuse split vertices at same coordinates #5985

Draft
wants to merge 3 commits into
base: dev-2.x
Choose a base branch
from

Conversation

hbruch
Copy link
Contributor

@hbruch hbruch commented Jul 25, 2024

Summary

This PR prevents recreating multiple permanent split vertices at the same coordinate.

Note: this fix is an alternative to #5956, which tries to prevent u-turns by adding costs for u-turns.

Issue

As described in #5955, linking currently creates multiple split vertices for forward and backward edges.
This PR lets VertexLinker keep track of split vertices during graph build and reuses an existing split vertex, if it has the same coordinates as the split coordinates for a different edge.

Closes #5955

Unit tests

A couple of test cases testing the number of created vertices/edges were adapted to reflect the reduced count of edges/nodes.

In addition, this PR provides a test case to illustrate #5955.

@hbruch hbruch requested a review from a team as a code owner July 25, 2024 11:04
Copy link

codecov bot commented Jul 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.64%. Comparing base (de56d00) to head (66534b5).

Additional details and impacted files
@@            Coverage Diff             @@
##             dev-2.x    #5985   +/-   ##
==========================================
  Coverage      69.64%   69.64%           
- Complexity     17164    17168    +4     
==========================================
  Files           1943     1943           
  Lines          73795    73808   +13     
  Branches        7552     7554    +2     
==========================================
+ Hits           51394    51406   +12     
+ Misses         19772    19771    -1     
- Partials        2629     2631    +2     

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

@optionsome optionsome requested a review from abyrd July 30, 2024 09:06
@leonardehrenfried
Copy link
Member

We talked about your PRs today and we would like you to call into one of the Tuesday meetings (where @abyrd tends to be present) to discuss the various approaches.

Personally I'm in favour of treating back and forward edges as a pair so that we can simply reuse the same splitter vertex in the relevant method without having to go through an index. Andrew might have other ideas as well.

@leonardehrenfried
Copy link
Member

I will convert this to a draft. You can convert it back to a PR when you start working on it again.

@leonardehrenfried leonardehrenfried marked this pull request as draft September 3, 2024 12:46
@t2gran t2gran added this to the 2.7 (next release) milestone Sep 18, 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.

U-turns not prevented at split edges
3 participants