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
Create a valid area even when it has too many vertices #5019
Create a valid area even when it has too many vertices #5019
Conversation
Area complexity check terminated area processin too early, leaving the area data invalid for linking.
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## dev-2.x #5019 +/- ##
=============================================
+ Coverage 64.49% 64.60% +0.11%
- Complexity 13863 13890 +27
=============================================
Files 1710 1710
Lines 67017 67054 +37
Branches 7203 7206 +3
=============================================
+ Hits 43221 43322 +101
+ Misses 21376 21312 -64
Partials 2420 2420
... and 13 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is good work, thanks @vesameskanen .
However since this code breaks a lot I think we really should be add tests for this behaviour. Personally, I would be ok if we read in real OSM data from Lund, run the area builder and assert a result. (https://docs.opentripplanner.org/en/dev-2.x/Preparing-OSM/#filtering-osm-data)
Since it's a bug that Skanetrafiken wants fixed, could we ask @Bartosz-Kruba and @jtorin to contribute the tests?
Indeed! Also, I have separately verified that it solves the problem we're seeing.
Lets discuss this on tomorrows dev-meeting. |
@jtorin Here is an example of how to extract data from a larger OSM file: https://github.com/leonardehrenfried/otp2-setup/blob/main/Makefile#L219 And here is an example for filtering out unneeded ways: https://docs.opentripplanner.org/en/dev-2.x/Preparing-OSM/#filtering-osm-data |
for (OSMNode nodeI : visibilityNodes) { | ||
sum_i += skip_ratio; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice fix!
Just a note that summing floats like this can actually be a bit dangerous. If skip_ratio is sufficiently small (like 1e-8) the sum will never reach 1. It probably won't be issue in this case but you might want to write this logic using an integer counter instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. In this case, float increment is typically greater than 0.1, and in some truly exceptional cases > 0.01. Lower values would mean that street graph is useless for routing, and it won't matter if area edges will not get added.
@vesameskanen I'm working on a test for |
Area complexity check terminated area processin too early, leaving the area data invalid for linking.
…g OpenStreetMapModule. Make Handler static, which means that it looses access to the fields in OSMModule. Solve this by a mix of passing copies of private values to a new instance, as well as funneling access to the data using Suppliers. A number of tests needs to adapt to the new extended contructor.
Uses OSM data from an area around Lund station, Sweden. Coverage as well as assertions are spotty and can be improved. With help from Leonard Ehrenfried.
…lex-area-fallback
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice that you got the tests in!
Just a comment about how we might be able to clean the code up a bit. But we could also just merge this as is as far as I'm concerned.
) { | ||
this.providers = List.copyOf(providers); | ||
this.boardingAreaRefTags = boardingAreaRefTags; | ||
this.graph = graph; | ||
this.issueStore = issueStore; | ||
this.areaVisibility = areaVisibility; | ||
this.banDiscouragedWalking = banDiscouragedWalking; | ||
this.banDiscouragedBiking = banDiscouragedBiking; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we add the staticParkAndRide
, staticBikeParkAndRide
, platformEntriesLinking
, customNamer
and maxAreaNodes
to the constructor as well? Then we could avoid the Supplier-thing and we could make the fields private. IMO it's better to have an obscenely big constructor than exposing public mutable fields.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately it's the other way around: the Supplier:s exists to couple the handler with the public fields in OpenStreetMapModule
. I checked in each public field and it was only banDiscouragedWalking
and banDiscouragedBiking
that wasn't modified externally, thus made them private final (immutable) and passable to Handler
.
Yes, this isn't especially pretty, but I tried to minimize the changes in the application code to avoid introducing bugs.
Since Handler is 1-to-1 instantiation-wise to the outer class OpenStreetMapModule
maybe they should be merged, and Handler
could be converted to an interface instead. Maybe WalkableAreaBuilder
and some of the code in OpenStreetMapModule
can be converted into smaller modules that are executed sequentially.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's true that the public fields are modified externally but from a quick glance it looked like all the modifications were made in tests that could trivially be changed to supply the values via constructor instead.
You're right that these classes would benefit from some refactoring :) But I guess that's out of scope for this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we all agree that these public mutable fields are terrible and we should get rid of it.
But frankly I feel like I've tortured @jtorin enough so I will not insist on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The best solution IMO would be to replace the OSMModule constructor with a builder so that the default values area set appropriately. I might do that in the future.
import org.opentripplanner.street.model.edge.AreaEdge; | ||
import org.opentripplanner.transit.model.framework.Deduplicator; | ||
|
||
public class WalkableAreaBuilderTest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good test and the beginning of something new - thanks!
I've taken the liberty to remove a bit of duplication by using a parameterized test. It's not really a requirement to getting this merged as it comes down to personal preferences, but if you like it, you can also cherry-pick this commit: leonardehrenfried@6eed582
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm approving the actual code changes, @habrahamsson-skanetrafiken has reviewed and approved my tests (as well).
Summary
Walkable area processing does not try to generate walking routes across the area, if the geometry is too complex. The complexity limit check terminated area processing so early that area data which is used in linking vertices inside the area was totally missing.
Now area processing initialises complex areas propely. Slow edge generation is mitigated by considering only maxAreaNodes subset from the visibility nodes.