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

Implement living street avoidance #2788

Merged
merged 10 commits into from
Feb 5, 2021
Merged

Conversation

SvetlanaBayarovich
Copy link
Contributor

@SvetlanaBayarovich SvetlanaBayarovich commented Jan 20, 2021

Issue

Introduces use_living_streets parameter which allows to tune living streets preferences during routing with different costings.
The impact on route selection consists of 2 parts:

  • penalize entering living street areas, if parameter value is in range [0, 0.5]
  • increase or decrease costs for living street edges according to parameter value.

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

Requirements / Relations

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

@SvetlanaBayarovich SvetlanaBayarovich self-assigned this Jan 20, 2021
"Place de Thorigny", "Rue de Thorigny",
"Rue Debelleyme", "Rue de Turenne",
"Rue Commines", "Rue Amelot"};
std::vector<std::string> expected_route{"Rue de la Perle", "Rue des Archives", "Rue Pastourelle",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rue de Thorigny is a living street, so avoid it now

Copy link
Member

Choose a reason for hiding this comment

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

but this is a bike share route which means its just pedestirans and bike riders, we shouldnt be avoiding living streets for those modes of travel should we?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, but this is the test case for auto costing, as the line 270 says, so I thought this was expected

@SvetlanaBayarovich SvetlanaBayarovich force-pushed the bsv-living-street-penalty branch 3 times, most recently from 6a2f4c0 to a9b11c9 Compare January 27, 2021 18:49
@SvetlanaBayarovich
Copy link
Contributor Author

Some test cases from initial issue:

prod result data
image image image
image image image

src/sif/dynamiccost.cc Outdated Show resolved Hide resolved
// 'living_street' edge
validate_path(gurka::route(use_living_streets_map, "1", "2", c),
{"AB", "BC", "CD", "DE", "EF"});
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This test is great, easy to read and follow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All credit goes to @genadz and his tests on track avoidance :)

purew
purew previously approved these changes Jan 27, 2021
Copy link
Contributor

@purew purew left a comment

Choose a reason for hiding this comment

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

Nice, LGTM

@SvetlanaBayarovich SvetlanaBayarovich marked this pull request as ready for review February 1, 2021 18:01
@@ -59,7 +59,8 @@ constexpr float kDefaultSideWalkFactor = 1.0f; // Neutral value for sidewalk
constexpr float kDefaultAlleyFactor = 2.0f; // Avoid alleys
constexpr float kDefaultDrivewayFactor = 5.0f; // Avoid driveways
constexpr float kDefaultUseFerry = 1.0f;
constexpr float kDefaultUseTracks = 0.5f; // Factor between 0 and 1
constexpr float kDefaultUseTracks = 0.5f; // Factor between 0 and 1
constexpr float kDefaultUseLivingStreets = 0.6f; // Factor between 0 and 1
Copy link
Member

Choose a reason for hiding this comment

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

should this be 1.f? isnt the point of most of these living streets that pedestrians get right of way at or above other modes of travel?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with your idea, but in my opinion pedestrians are more sensitive to route distance/duration increase which can happen if we favor living streets more. That's why I didn't want to mess with this costing a lot

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting, so a customer who would like more scenic and clean routes may want to crank this up at the potential expense of time and distance. Makes sense.

@SvetlanaBayarovich
Copy link
Contributor Author

According to the testing, the amount of routes that changed significantly is relatively small. There are only 62 routes with relative difference in distance greater than 1%. As for the weight, the routes with high difference are all of a certain type: short (from 300-400 meters to couple of kilometers) and with mandatory living street edges on them, so that the route itself does not change even change wrt master, only the cost increases. Thus when living streets are not mandatory, we manage to avoid them without significant weight increase.

The most interesting cases I came across are

1)Well, we do avoid this living street. The result just does not look nice.

master living street avoidance data
image image image

2)More relevant for the large areas of living streets. We add sth like 400 meters to the total distance to save like ~50 meters of riding living streets (because 'living street' edges are really costly)

master living street avoidance data
image image image

gknisely
gknisely previously approved these changes Feb 3, 2021
@SvetlanaBayarovich
Copy link
Contributor Author

Testing another approach with high penalty for transition into living street and not such a big factor (previously I was betting everything on large factor) in order to avoid situation 2) from my comment above. The idea is to minimize the part of the route that goes through living streets, but not to add to much to the route length if we need to use it as an origin or destination

@gknisely gknisely merged commit cdbec05 into master Feb 5, 2021
@SvetlanaBayarovich SvetlanaBayarovich deleted the bsv-living-street-penalty branch May 4, 2021 14:20
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.

None yet

4 participants