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

Probable restrictions #3361

Merged
merged 37 commits into from
Oct 21, 2021
Merged

Probable restrictions #3361

merged 37 commits into from
Oct 21, 2021

Conversation

gknisely
Copy link
Member

@gknisely gknisely commented Oct 18, 2021

Issue

This PR consumes and utilizes probable route restrictions. The lowest probability allowed is set in the costing. Currently, only using probable restrictions that have a 100% probability.

Tasklist

Requirements / Relations

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

@@ -2091,8 +2091,14 @@ function rels_proc (kv, nokeys)
end

if (kv["type"] == "route" or kv["type"] == "restriction") then
if kv["restriction:probable"] then
if kv["restriction"] or kv["restriction:conditional"] then
kv["restriction:probable"] = nil
Copy link
Member Author

Choose a reason for hiding this comment

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

a user entered restriction always wins over a probable one

constexpr float kDefaultUseTolls = 0.5f; // Default preference of using toll roads 0-1
constexpr float kDefaultUseTracks = 0.f; // Default preference of using tracks 0-1
constexpr float kDefaultUseDistance = 0.f; // Default preference of using distance vs time 0-1
constexpr uint32_t kDefaultProbability = 100; // Default percentage of allowing probable restrictions
Copy link
Member Author

Choose a reason for hiding this comment

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

currently only care about restrictions with a probability of 100

@@ -410,6 +410,13 @@ class DynamicCost {
// Iterate through the restrictions
const EdgeLabel* first_pred = &pred;
for (const auto& cr : restrictions) {
if (cr->type() == baldr::RestrictionType::kNoProbable ||
cr->type() == baldr::RestrictionType::kOnlyProbable) {
if (probability_ == 0 || probability_ > cr->probablity()) {
Copy link
Member Author

Choose a reason for hiding this comment

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

never use a probable restriction that is 0

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not?

Copy link
Contributor

@purew purew Oct 19, 2021

Choose a reason for hiding this comment

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

Wouldn't that be the logical way to disable probable restrictions?

Copy link
Member Author

Choose a reason for hiding this comment

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

@purew yes that is exactly what this is doing. probability_ = 0 shuts off looking at any probable restrictions

Copy link
Contributor

Choose a reason for hiding this comment

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

Tiny nit, just saw the spelling issue with probablity -> probability

Copy link
Member Author

Choose a reason for hiding this comment

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

@purew 🦅 👁️

Copy link
Contributor

Choose a reason for hiding this comment

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

What if probabiity_==0 and cr->probability()==0? (This would be odd?)

Copy link
Member Author

Choose a reason for hiding this comment

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

@ktatterso probabiity_==0 would be true and we would just continue

Copy link
Member Author

Choose a reason for hiding this comment

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

@ktatterso I forgot to mention that we never have a complex restriction with a probability of 0. It means you have no faith in the probable restriction at all

Copy link
Contributor

Choose a reason for hiding this comment

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

@gknisely Thanks, I guess I meant the second part, what does cr->probability()==0 mean? And you just answered it. 🙂

@@ -907,6 +917,8 @@ class DynamicCost {
destination_only_penalty_ = costing_options.destination_only_penalty();
maneuver_penalty_ = costing_options.maneuver_penalty();

probability_ = costing_options.probability();
Copy link

Choose a reason for hiding this comment

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

To re-confirm this is the logic that allows the probability threshold to be passed through in the payload under the costing_options key, right?

"costing_options": {"auto": {"probability": prob}}

Where prob is some values 0-100.

This means that, even though we default to 100, we can, when running Valhalla ourselves, pass in alternative thresholds when testing new PRRs/thresholds.

(cc @denmoroz)

Copy link
Member Author

Choose a reason for hiding this comment

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

@kuanb this is correct

* Get the probability for the restriction.
* @return Returns the probability for this restriction.
*/
uint64_t probablity() const {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a value between 0 and 100 that means percentage? If so could this be mentioned in doc-string and return type be set to uint8_t?

Copy link
Member Author

Choose a reason for hiding this comment

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

@purew all set.

* Set the probability for the restriction.
* @param probability probability for this restriction.
*/
void set_probability(const uint64_t probability) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as above, doc-string should mention what unit this is and input type should likely be shrunk to uint8_t.

Copy link
Member Author

Choose a reason for hiding this comment

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

@purew looks like a lot of the set and gets need to be fixed. Let me fix them.

Copy link
Member Author

Choose a reason for hiding this comment

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

@purew done

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.

Left some more comments

@@ -76,6 +78,7 @@ constexpr ranged_default_t<float> kUseTollsRange{0, kDefaultUseTolls, 1.0f};
constexpr ranged_default_t<float> kUseDistanceRange{0, kDefaultUseDistance, 1.0f};
constexpr ranged_default_t<float> kAutoHeightRange{0, kDefaultAutoHeight, 10.0f};
constexpr ranged_default_t<float> kAutoWidthRange{0, kDefaultAutoWidth, 10.0f};
constexpr ranged_default_t<float> kProbabilityRange{0, kDefaultProbability, 100};
Copy link
Contributor

Choose a reason for hiding this comment

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

The probability elsewhere is a uint8_t, should this kProbabilityRange be the same?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes copy & paste fail

Copy link
Member Author

Choose a reason for hiding this comment

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

@purew I made this uint32_t to be consistent with the other options https://github.com/valhalla/valhalla/blob/master/proto/options.proto

// probability
pbf_costing_options->set_probability(
kProbabilityRange(rapidjson::get_optional<uint32_t>(*json_costing_options, "/probability")
.get_value_or(kDefaultProbability)));
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we sanity check the option from the user, check that it is between 0 and 100 since it comes from an end-user?

Copy link
Member Author

Choose a reason for hiding this comment

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

@purew no need. this checks it for us constexpr ranged_default_t<uint8_t> kProbabilityRange{0, kDefaultProbability, 100};

{{"/date_time/type", std::to_string(GetParam())},
{"/date_time/value", "2021-04-02T19:20"}});
gurka::assert::raw::expect_path(result, {"AB", "BC", "CF"});
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a test where the probable restriction value is set as part of the request?

Copy link
Member Author

Choose a reason for hiding this comment

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

@purew sure. will do

Copy link
Member Author

Choose a reason for hiding this comment

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

@purew done

@ktatterso
Copy link
Contributor

Seems like OSMRestriction should have a ctor that memset's itself to all 0's (like OSMWay). Otherwise the member data is unset.

@@ -132,6 +132,7 @@ message CostingOptions {
optional bool include_hov2 = 74;
optional bool include_hov3 = 75;
optional bool exclude_cash_only_tolls = 76;
optional uint32 probability = 77;
Copy link
Member

@dgearhart dgearhart Oct 20, 2021

Choose a reason for hiding this comment

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

name needs to be more descriptive - something like: restriction_probability

constexpr float kDefaultUseTolls = 0.5f; // Default preference of using toll roads 0-1
constexpr float kDefaultUseTracks = 0.f; // Default preference of using tracks 0-1
constexpr float kDefaultUseDistance = 0.f; // Default preference of using distance vs time 0-1
constexpr uint32_t kDefaultProbability = 100; // Default percentage of allowing probable restrictions
Copy link
Member

Choose a reason for hiding this comment

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

naming again: kDefaultRestrictionProbability

Copy link
Member

@dgearhart dgearhart left a comment

Choose a reason for hiding this comment

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

made two naming change requests so it is more descriptive - other than that , it looks good to me

Copy link
Contributor

@merkispavel merkispavel left a comment

Choose a reason for hiding this comment

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

Check out my comment about default value/behaviour. I believe we can drop probability_ = 0 case from the code
#3361 (comment)

src/sif/autocost.cc Outdated Show resolved Hide resolved
merkispavel
merkispavel previously approved these changes Oct 20, 2021
Copy link
Contributor

@merkispavel merkispavel left a comment

Choose a reason for hiding this comment

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

Looks good 👍

Copy link
Contributor

@merkispavel merkispavel left a comment

Choose a reason for hiding this comment

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

🚢

@gknisely
Copy link
Member Author

Going to RAD / QT test one more time due to all the changes

Copy link
Member

@dgearhart dgearhart left a comment

Choose a reason for hiding this comment

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

If RAD testing looks good then 🚢

@gknisely
Copy link
Member Author

Seems like OSMRestriction should have a ctor that memset's itself to all 0's (like OSMWay). Otherwise the member data is unset.

@ktatterso going to have a small follow up pr for this as this has been like this for some time. Just don't want to mess up the functionality and this shouldn't, but you never know.

@gknisely gknisely merged commit feb722a into master Oct 21, 2021
@gknisely gknisely deleted the probable_restrictions branch February 22, 2024 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants