Skip to content

Do not re-link stops that were already linked by TransitToTaggedStopsModule - #2372

Closed
hannesj wants to merge 7 commits into
opentripplanner:dev-1.xfrom
hannesj:patch-1
Closed

Do not re-link stops that were already linked by TransitToTaggedStopsModule#2372
hannesj wants to merge 7 commits into
opentripplanner:dev-1.xfrom
hannesj:patch-1

Conversation

@hannesj

@hannesj hannesj commented Dec 2, 2016

Copy link
Copy Markdown
Contributor

No description provided.

@novalis

novalis commented Dec 30, 2016

Copy link
Copy Markdown
Contributor

I'm not really up on the latest state of OTP testing, but would it be possible to have some sort of test for this code?

@abyrd

abyrd commented Mar 14, 2017

Copy link
Copy Markdown
Member

@hannesj please supply a description for any pull request, including details of issue it solves or a link to the issue. Issue #2371 which appears above because it references this PR seems to be about a different issue, which happens to reference this PR in a comment.

I see that the message on the commit also says it fixes #2371, but these seem like different issues to me. @hannesj can you give a bit more information? If I merge this commit we may have an inaccurate message in the git log, and the issue would be closed by a possibly unrelated commit.

@abyrd abyrd self-assigned this Mar 14, 2017
@hannesj

hannesj commented Dec 7, 2017

Copy link
Copy Markdown
Contributor Author

@abyrd Sorry for the long delay.

The main issue which this PR solves is, if a stop is already linked by the TransitToTaggedStopsModule, then it would be linked multiple times to the street graph, which would enable shortcuts through the stop nodes. Also, if we already have the correct position form OSM, then it would link the node to a more inferior place in the street graph.

@abyrd abyrd changed the title Disable linking from already linked stops Do not re-link stops that were already linked by TransitToTaggedStopsModule Sep 11, 2018
abyrd added 5 commits July 4, 2019 14:05
This was done slightly out of order - I accidentally performed release
1.4.0 off the dev-1.x branch before merging to master, rather than
merging dev-1.x to master and then releasing from master.
evansiroky
evansiroky previously approved these changes Sep 6, 2019

@evansiroky evansiroky left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

With my recent PR of #2817 and improvements to PR #2762 there was some revival of the discussion in #2371, so I decided to look at this PR to see what it might do. This PR makes sense in either the case where the double StreetTransitLink (STL) traversal even for same stops was allowed or with the #2817 implementation. I don't think allowing a double STL traversal to the same stop is a good idea and so I think #2817 is the right way to go. But even with #2817, there will still be duplicate STLs created when the TransitToTaggedStopsModule is enabled. Those duplicate STLs won't cause computational incorrectness, but as noted the street splitting of the stops could result in inferior insertion into OSM. Furthermore, not adding them as proposed here would result in slightly faster queries due to not traversing unneeded edges.

@t2gran t2gran added X OTP1 ~ Not in use any more ~ Fix or backport to the 1.x version of OTP OTP2 labels Sep 13, 2019
t2gran
t2gran previously approved these changes Sep 13, 2019
@t2gran
t2gran requested a review from a team September 13, 2019 16:49
@t2gran
t2gran dismissed stale reviews from evansiroky and themself via f303f3c October 13, 2019 15:47
@t2gran
t2gran changed the base branch from master to dev-1.x October 13, 2019 15:47
@t2gran

t2gran commented Oct 13, 2019

Copy link
Copy Markdown
Member

This PR base was the master branch , so to apply it on the new dev-1.x and dev-2.x branches I had to cherry pick the commit (only 1) onto the target branches. Changing the base did not work, because other commits from the master would then be carried over.

This is the manual cherry-picked commits, witch completes this PR:

@abyrd abyrd added this to the 2.0 milestone Oct 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

X OTP1 ~ Not in use any more ~ Fix or backport to the 1.x version of OTP

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants