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.

Sign up for GitHub

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

Jump to bottom

Fix ferry shortest path recovery #4378

Merged
merged 16 commits into from
Nov 15, 2023
Merged

Fix ferry shortest path recovery #4378

merged 16 commits into from
Nov 15, 2023

Conversation

dnesbitt61
Copy link
Member

@dnesbitt61 dnesbitt61 commented Nov 2, 2023 •
edited
Loading

fixes #4373
fixes #4358

Incorrect paths are frequently recovered from the ShortestPath method when reclassifying edges approaching ferries. This fixes the last_label_idx and ensures it is only set when the proper classification is reached or exceeded.

If no edges are encountered with proper classification, then no edges are reclassified. A log statement was added to identify lat,lng locations where no edges are reclassified.

For a test case of New York state I see the following for ferries that failed to find paths to the target road classification:

2023/11/03 14:58:22.177548 [INFO] Reclassification fails both directions to ferry at LL =40.693268,-74.015295
2023/11/03 14:58:22.306188 [INFO] Reclassification fails both directions to ferry at LL =41.172929,-72.204647
2023/11/03 14:58:22.317576 [INFO] Reclassification fails both directions to ferry at LL =41.256999,-72.031202
2023/11/03 14:58:22.448317 [INFO] Reclassification fails both directions to ferry at LL =43.911944,-74.919672
2023/11/03 14:58:22.456406 [INFO] Reclassification fails both directions to ferry at LL =44.553565,-73.855213

These all seem valid.

Tasklist

  • Add tests
  • Add #fixes with the issue number that this PR addresses
  • Update the docs with any new request parameters or changes to behavior described
  • Update the changelog
  • If you made changes to the lua files, update the taginfo too.

Requirements / Relations

Link any requirements here. Other pull requests this PR is based on?

@dnesbitt61
Add test that demonstrates the wrong path can be recovered from the
2fdda12 
ShortestPath method in ferry_connections.cc
@dnesbitt61
Fix the last_label_idx - make sure it is only set when an edge with the
095cfbe 
target classification (or better) is present at the node being expanded.
Identify cases where no reclassification occurs (e.g,. islands with no
edges that meet the target classification) and log the locations.
@dnesbitt61
Copy link
Member Author

Looking for ideas on a better "termination" - identifying when "enough" edges of the proper classification are found.

@nilsnolde
Copy link
Member

Should we add a small test for #4358 ? i.e. a map which doesn't have any edge >= kPrimary and make sure we didn't reclassify anything?

@dnesbitt61
If no path is found, always add to set of ferry locations with no path.
6c07ab5
@dnesbitt61
Add test to make sure no edges are reclassified if the target
8bb7f99 
classification is not found.
@dnesbitt61
Looks like ReclassifyFerryConnections automatically sets the first edge
1f06a9e 
connecting to the ferry to primary. I added a test to make
sure no edges are reclassified if no edges with the target
classification are encountered. However, the initial ferry connection
edge is set to primary even if no edges with the target classification
are encountered in ShortestPath.
@dnesbitt61
Copy link
Member Author

I added such a test. It looks like the first connecting edge is always reclassified (see comment close to the bottom of ferry_connections.cc).

@nilsnolde
Copy link
Member

nilsnolde commented Nov 2, 2023 •
edited
Loading

It looks like the first connecting edge is always reclassified

Hmpf, yeah, maybe we should do this here:

        update_edge.attributes.importance = total_count > 0 ? kFerryUpClass : update_edge.attributes.importance;

?

Or smth like that, didn't check with your changes..

@nilsnolde nilsnolde mentioned this pull request Nov 3, 2023
Open
@dnesbitt61
Formatting
9bf4fc3
@dnesbitt61
remove some method declarations - not needed since only used in
615cd08 
ferry_connections.cc.
@dnesbitt61
More properly identify cases where no paths to/from ferries to target
7431290 
classification roadways exist.
@dnesbitt61
formatting
928d111
@dnesbitt61
don't need to include unordered_set anymore.
1007c6f
@dnesbitt61
Remove the check if node_labels.size == 0. This is redundant now that we
a0bd237 
check for a valid last_label_idx being set.
@dnesbitt61
Merge branch 'master' into fix_ferry_shortestpath
2f7bbbb
@dnesbitt61
Reclassify the first edge IF a connection to higher class roads is
4706189 
found.
@dnesbitt61
Merge remote-tracking branch 'origin/fix_ferry_shortestpath' into fix…
19b65ae 
…_ferry_shortestpath
@dnesbitt61
Modify the nothing reclassified test to remove the first edge connected
6aae8c6 
to the ferry. No longer upclassed if no path to higher class edges is
found.
nilsnolde
nilsnolde approved these changes Nov 13, 2023
View reviewed changes
Copy link
Member

@nilsnolde nilsnolde left a comment

Choose a reason for hiding this comment

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

LGTM 🚢

@dnesbitt61 dnesbitt61 merged commit 274a3e7 into master Nov 15, 2023
6 of 7 checks passed
@dnesbitt61 dnesbitt61 deleted the fix_ferry_shortestpath branch November 15, 2023 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nilsnolde nilsnolde nilsnolde approved these changes

@kevinkreiser kevinkreiser Awaiting requested review from kevinkreiser

@gknisely gknisely Awaiting requested review from gknisely

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@dnesbitt61 @nilsnolde