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

Penalize private gates #3144

Merged
merged 4 commits into from
Jun 22, 2021
Merged

Penalize private gates #3144

merged 4 commits into from
Jun 22, 2021

Conversation

CuriOusQuOkka
Copy link
Contributor

@CuriOusQuOkka CuriOusQuOkka commented Jun 14, 2021 •
edited
Loading

Issue

Apply penalty to the gates that are marked as private. The penalty is set higher than the penalty for regular gates.

Tasklist

  • Add tests
  • 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

Continue this task 0488133

@CuriOusQuOkka CuriOusQuOkka force-pushed the penalize_private_gates branch from 199524c to 5cb05c1 Compare June 14, 2021 13:37
gknisely
gknisely reviewed Jun 14, 2021
View reviewed changes
test/parse_request.cc Outdated
@@ -104,6 +110,8 @@ constexpr float kDefaultBicycle_DrivewayPenalty = 300.0f; // Seconds
constexpr float kDefaultBicycle_AlleyPenalty = 60.0f; // Seconds
constexpr float kDefaultBicycle_GateCost = 30.0f; // Seconds
constexpr float kDefaultBicycle_GatePenalty = 600.0f; // Seconds
constexpr float kDefaultBicycle_PrivateAccessCost = 5.0f; // Seconds
constexpr float kDefaultBicycle_PrivateAccessPenalty = 750.0f; // Seconds
Copy link
Member

Choose a reason for hiding this comment

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

@CuriOusQuOkka why is kDefaultBicycle_PrivateAccessPenalty 750 for bike but 450 for the other costing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The values are temporary now, I'm working on tuning them. But in the beginning it was set so high, to have kDefaultBicycle_GatePenalty < kDefaultBicycle_PrivateAccessPenalty. As we assume that private gates should be penalized more than gates with no access information.

Copy link
Member

Choose a reason for hiding this comment

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

I generally use a lower gate cost these days for bicycles so perhaps it can be lowered.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I'll try it out.

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've set default gate_penalty to 300 for bikes, tested routes didn't change. private_access_penalty=450 looks good for bikes.

@CuriOusQuOkka CuriOusQuOkka force-pushed the penalize_private_gates branch from fc91005 to 3714218 Compare June 17, 2021 14:32
genadz
genadz previously approved these changes Jun 18, 2021
View reviewed changes
@CuriOusQuOkka CuriOusQuOkka force-pushed the penalize_private_gates branch 2 times, most recently from d5a6551 to adcf9d2 Compare June 18, 2021 15:39
@CuriOusQuOkka CuriOusQuOkka marked this pull request as ready for review June 18, 2021 16:10
@CuriOusQuOkka CuriOusQuOkka requested a review from gknisely June 21, 2021 13:10
@CuriOusQuOkka CuriOusQuOkka force-pushed the penalize_private_gates branch from adcf9d2 to 2c82ee8 Compare June 21, 2021 18:01
@CuriOusQuOkka CuriOusQuOkka force-pushed the penalize_private_gates branch from 2c82ee8 to ef8a0a7 Compare June 21, 2021 18:11
@CuriOusQuOkka CuriOusQuOkka requested a review from genadz June 21, 2021 18:16
@gknisely
Copy link
Member

@CuriOusQuOkka can we see some before and after screen shots of what is changing? I assume this was RAD tested?

@CuriOusQuOkka
Copy link
Contributor Author

@CuriOusQuOkka can we see some before and after screen shots of what is changing? I assume this was RAD tested?

Made a huge RAD testing for this task and no problems were caught. Here are examples of what've changed

  1. Used regular gate instead of private,
    image

  2. Do not go through private gate. Didn’t work earlier because access was given (private),
    image

  3. Took the route with 1 private gate instead of 2, image

gknisely
gknisely approved these changes Jun 21, 2021
View reviewed changes
Copy link
Member

@gknisely gknisely 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 to me. @dnesbitt61 thoughts

@CuriOusQuOkka CuriOusQuOkka requested a review from dnesbitt61 June 21, 2021 22:33
dnesbitt61
dnesbitt61 approved these changes Jun 22, 2021
View reviewed changes
Copy link
Member

@dnesbitt61 dnesbitt61 left a comment

Choose a reason for hiding this comment

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

Does private_access_penalty only apply to gates? If so maybe the name should include gates within it? I just wonder if there will be other places where transitioning onto private access might be penalized.
Otherwise I approve.

@CuriOusQuOkka
Copy link
Contributor Author

Does private_access_penalty only apply to gates? If so maybe the name should include gates within it? I just wonder if there will be other places where transitioning onto private access might be penalized.
Otherwise I approve.

@dnesbitt61 It is used for gates now, but we plan to include this penalty for bollards soon.

@CuriOusQuOkka CuriOusQuOkka merged commit b91d64f into master Jun 22, 2021
CuriOusQuOkka added a commit that referenced this pull request Jun 24, 2021
@CuriOusQuOkka
Penalize private gates (#3144)
3b18efb 
* penalize private gates
CuriOusQuOkka added a commit that referenced this pull request Jun 24, 2021
@CuriOusQuOkka
Penalize private gates (#3144)
4f5d069 
* penalize private gates
@CuriOusQuOkka CuriOusQuOkka deleted the penalize_private_gates branch December 18, 2021 00:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gknisely gknisely gknisely approved these changes

@dnesbitt61 dnesbitt61 dnesbitt61 approved these changes

@kevinkreiser kevinkreiser Awaiting requested review from kevinkreiser

@genadz genadz Awaiting requested review from genadz

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@CuriOusQuOkka @gknisely @dnesbitt61 @genadz