-
Notifications
You must be signed in to change notification settings - Fork 232
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
Tiered Dwell Time with Rates #658
Tiered Dwell Time with Rates #658
Conversation
…tch'. Add example policies using rate properties with time rules.
I think that this should also address #633 |
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.
Thanks for the examples! Hoping for some clarification on how to interpret maximum
and minimum
and about some assumptions when processing rules. I've bolded some places where I was making assumptions that I'm not sure are documented.
"geographies": ["0c77c813-bece-4e8a-84fd-f99af777d198"], | ||
"statuses": { "available": [], "non_operational": [] }, | ||
"vehicle_types": ["bicycle", "scooter"], | ||
"maximum": 2, |
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 you explain how maximum
is interpreted here? And conversely, how does one interpret minimum
? I know we talked about this on a call and I think I kind of understood it then, but I've forgotten now... 😬 I think this needs some clarifying documentation or a more clear name.
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.
Maximum in this case would be "If a vehicle surpasses this maximum, it's matched by this rule". Similarly, a minimum would be "If a vehicle is under this minimum, it's matched by this rule". I can't think of any good real minimum
time policies off the top of my head, but essentially you could encode something like "Scooters must be parked here in state x for a minimum of 1 hour prior to doing anything else". Maybe this could be used to encode some rules around off-hours usage perhaps?
You can think of the matching along the lines of:
if (time_since_last_event >= maximum) {
// maybe a match, check other criteria (geos, states, etc...)
} else if (time_since_last_event =< minimum) {
// maybe a match, check other criteria (geos, states, etc...)
} else {
definitely not a match
}
"prev_policies": null, | ||
"provider_ids": [], | ||
"currency": "USD", | ||
"rules": [ |
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 want to make sure I understand the process for interpreting these rules, so I'm going to walk through an example. Let's say we have a 6.1 hour long parking event. The total charge for that under this policy as I understand it would be 2 + 4 + 10 * 5 = $56, right?
So the excruciatingly detailed process for calculating that would be:
- Iterate over rules
- Looking at the first rule:
- This is an
each_time_unit
rule, so I need to figure out how many units of time to apply it to - The rule is in units of hours, so I make sure my duration is also in hours, or in some other way normalize the units of the rule and my event
- This rule has a
maximum
of 2 and my event is longer than that, so this rule applies to my event - This rule has a
maximum
of 2 so I subtract 2 from 6.1 to get 4.1 hours of this event this rule applies to- I don't understand how the word
maximum
describes these last two operations, hoping for some clarification on that
- I don't understand how the word
each_time_unit
rules are always applied to whole time units, never fractional, so I round 4.1 up to 5 to get the total number of hours used to calculate this portion of the charge- Is this assumption written down anywhere?
- The
rate_amount
for this rule is 1000, I multiple 5 hours * 1000 = 5000 cents = $50 - Having accounted for this portion of the parking event, I only carry forward the portion of this parking event that is less than the
maximum
field, meaning the first 2 hours. I replace my original parking event with one that is 2 hours long as I head to the next rule. This is standard practice foreach_time_unit
rules.
- This is an
- Looking at the second rule now that I have a 2 hour parking event:
- This is an
each_time_unit
rule, so I need to figure out how many units of time to apply it to - The rule is in units of hours, so I make sure my duration is also in hours, or in some other way normalize the units of the rule and my event
- This rule has a
maximum
of 1 and my event is longer than that, so this rule applies to my event - This rule has a
maximum
of 1 so I subtract 1 from 2 to get 1 hour of this event this rule applies to each_time_unit
rules are always applied to whole time units, never fractional, so I round 1 up to 1 to get the total number of hours used to calculate this portion of the charge (hoping there's no floating point issues causing the rounding to go to 2)- The
rate_amount
for this rule is 400, I multiple 1 hour * 400 = 400 cents = $4 - Having accounted for this portion of the parking event, I only carry forward the portion of this parking event that is less than the
maximum
field, meaning the first 1 hour. I replace my original parking event with one that is 1 hour long as I head to the next rule.
- This is an
- Looking at the third rule now that I have a 1 hour parking event:
- This is an
each_time_unit
rule, so I need to figure out how many units of time to apply it to - The rule is in units of hours, so I make sure my duration is also in hours, or in some other way normalize the units of the rule and my event
- This rule has a
maximum
of 0 and my event is longer than that, so this rule applies to my event - This rule has a
maximum
of 0 so I subtract 0 from 1 to get 1 hour of this event this rule applies to each_time_unit
rules are always applied to whole time units, never fractional, so I round 1 up to 1 to get the total number of hours used to calculate this portion of the charge (hoping there's no floating point issues causing the rounding to go to 2)- The
rate_amount
for this rule is 200, I multiple 1 hour * 200 = 200 cents = $2 - Having accounted for this portion of the parking event, I only carry forward the portion of this parking event that is less than the
maximum
field, meaning 0 hours. - Having applied rules to the entirety of my parking event (left with one 0 hours long) I know that no more rules will apply to my event and I can break out of the iteration over rules.
- Is this true? Could there be another rule after this one with a flat fee or something?
- This is an
- I sum up the charges from each rule and get $56 total
Does that seem right? If one of these rules had a minimum
field, how would that factor in?
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 that the methodology you described functionally works how I'd expect at the end of the day, and the logic checks out! However, I have perhaps a slightly more intuitive way of thinking about it. The most intuitive way I can think of evaluating policies with rates that specify each_time_unit
, is by treating each_time_unit
tick as an event (note, not an MDS event, more of an event-driven-system kind of event). This would enable continuous charging throughout the duration of the parking, as opposed to waiting till the absolute duration of the park has been completed.
Let's, for the sake of making this a bit easier to reason about, assume that the time_unit
for each rule is the same, so we can evaluate the whole policy at once every hour on the dot. Because this is each_time_unit
instead of each_complete_time_unit
, we will evaluate on first entry in addition to after each hour ticks.
Continuous(ish) Evaluation Methodology for this Policy
I'm gonna approach this with some brevity here, but if you'd like me to dial into some more detail please let me know and I'm happy to!
@00:00 (right when first parked)
- First rule doesn't match, continue
- Second rule doesn't match, continue
- Third rule matches, charge $2
@01:00
- First rule doesn't match, continue
- Second rule matches, charge $4, short-circuit execution
@02:00
- First rule matches, charge $10, short-circuit execution
@03:00
- First rule matches, charge $10, short-circuit execution
@04:00
- First rule matches, charge $10, short-circuit execution
@05:00
- First rule matches, charge $10, short-circuit execution
@06:00
- First rule matches, charge $10, short-circuit execution
At the end of the day, there will be a charge for $2 + $4 + $10 + $10 + $10 + $10 + $10 = $56.
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.
In order for this to work the the comparison to maximum
looks like it needs to be >=
? And same for minimum
?
If you did have different units in the rules would you pre-process the rules into the same units in order to figure out your clock-ticks?
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, >=
for maximums, and similarly <=
for minimums I think (let me update my other comment, I think I didn't include the =
component).
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.
If I had different units on the rules I'd probably specify an interval to re-evaluate of the lowest-duration unit referenced (e.g. if there were some rules with minutes and some with hours, I'd evaluate on a minutely basis). I'd then probably add some logic to ensure that double-fees won't get charged if a high-duration rule is hit again within that unit of its last occurrence, and instead results in a short-circuit without a charge.
if (current_timestamp - prev_fee_timestamp < matched_rule.unit) {
// don't charge anything, because we don't want to double charge
}
"prev_policies": null, | ||
"provider_ids": [], | ||
"currency": "USD", | ||
"rules": [ |
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 want to make sure I understand the process for interpreting these rules, so I'm going to walk through an example. Let's say we have a 1.25 hour long parking event. The total charge for that under this policy would be $4.
So the excruciatingly detailed process for calculating that would be:
- Iterate over rules
- Looking at the first rule:
- This is a
once_on_unmatch
rule so I need to figure out if I have parking event that has gone from matching this rule to not matching this rule - This rule has
rule_units
of hours and I normalize my event duration with the rule's units - The rule has a
maximum
of 2, which is more than the duration of my event, so I move on to the next rule
- This is a
- Looking at the second rule:
- This is a
once_on_unmatch
rule so I need to figure out if I have parking event that has gone from matching this rule to not matching this rule - This rule has
rule_units
of hours and I normalize my event duration with the rule's units - This rule has a
maximum
of 1. Aonce_on_unmatch
rule always applies when my event is longer than a rule'smaximum
, so I need to use this rule to calculate a fee. - The fee for this rule is 400 = $4
once_on_unmatch
rules always apply to the entirety of a parking event and once I've applied rules to my entire event I can stop iterating over rules, even if there are more, so I break out of the loop over rules.- Is this true? Could there be more applicable rules, like rate rules? Or would those be in a different policy?
- This is a
- I sum up the charges for each rule for a total of $4
Does that all seem right?
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.
All sounds right to me! Short-circuiting should behave exactly how you described.
Would changing |
@jiffyclub Perhaps changing the terminology would make it a bit more intuitive, yeah. I think that Regardless, any nomenclature change for this would have to be a breaking change, so we should probably start a Discussion thread to talk about it for the 2.0 cycle! |
I'm looking for documentation or an example somewhere that specifies that a rule's rate applies when an event falls outside the bounds of the rule's specified minimum and maximum, but I don't see that anywhere. Do you know where that comes from? Could the convention instead be that rates apply when an event falls inside the rules minimum and maximum values? For all other fields (geography, state, vehicle type, etc) the convention is to use a positive match, so it seems odd that for minimum and maximum we use a negative match. Separate point: the new examples should be added to the policy examples README. |
Good point @jiffyclub , I suppose it's not clarified anywhere in the spec that Say you want to express a law: "You're only allowed to park here for 2 hours or you get towed". Is it more intuitive to express that as What about for
|
The towing example I think is more intuitively expressed with a minimum because it's a positive association, e.g. "if you are parked for a minimum of 2 hours you will be towed" vs "if you are parked for a maximum of two hours you will not be towed". In the latter the "not" is doing a lot of work, which is not as clear as it could be. For the caps and speed limit I see what you mean, having the monetary calculation tied to being outside the range saves having to define rules for the parts outside of the range. It would be more explicit to do so, but more work and more maintenance. Though again these rely on a negative association, which I think is generally less clear than a positive association. But consider a policy in which a city subsidizes the first 500 vehicles an operator puts out. That would need to be expressed with I wonder if there's a way to make explicit which way min and max are to be treated, like with a flag. |
These changes seem ok to me, but I'm not 100%. Could those of you who have commented and are interested go back and resolve each conversation, and call out things that need more community input? |
My overall thoughts on merging: I like this concept of pairing rate information with non-rate rules. In fact, I don't think policy should even have rate rules (see #662). I'd like to see this merged in conjunction with some other improvements:
|
@avatarneil will be presenting on this in today's Working Group meeting. |
Is everyone ok with this as written (@avatarneil @marie-x @jean-populus @jiffyclub) and does it address both #633 and #631 to your liking? If so I will approve and merge this week. |
Seeing no objections to this, I'm merging it to 'dev' for inclusion in MDS 1.2.0. Thanks for your work on this. |
I'm ok with this merged, but without resolving the issues I mentioned above policies still have ambiguous interpretations. (#658 (comment)) |
Can someone do a PR to resolve the ambiguous policy issues? |
I'll open some today. |
Explain pull request
There has been a lot of discussion recently around supporting duration based fees in MDS Policy (see #631), but I'll try to put a short explanation in this PR.
Say, for example, a city wishes to have a parking zone which costs $2 for the first hour, $4 for the second hour, and $10 for every hour onwards (note, these exemplar parking fees are rather high 😅 ); at the moment, this is not supported by the
rate
rule type. Luckily, MDS Policy has thetime
rule type, which can perfectly handle this case! We can leverage Policy's short-circuiting behavior by creatingtime
rules which start off with higher time ranges, and increasingly slice down into lower ranges. In this particular example, we could order our rules as such:All of these rules would have a
rate_recurrence
ofeach_time_unit
, such that if I was parked for a total of 2.5hrs I'd be charged $2 + $4 + $10 = $16.This PR also takes a stab at addressing another type of duration based fee, where there is a fee is charged based on the total time parked, as opposed to additively per hour. It spins the
rate_recurrence: 'once'
type out intoonce_on_match
andonce_on_unmatch
(open to suggestions for a better contronym here!). In the example for this policy, the rules are structured the same way, however therate_recurrence
is set toonce_on_unmatch
, such that if I was parked for a total of 2.5hrs I'd be charged $10, and if I was parked for 4.5hrs I'd still be charged $10!Is this a breaking change
A breaking change would require consumers or implementors of the API to modify their code for it to continue to function (ex: renaming of a required field or the change in data type of an existing field). A non-breaking change would allow existing code to continue to function (ex: addition of an optional field or the creation of a new optional endpoint).
The change from
once
toonce_on_match
could be considered a breaking change, however because Rates are in beta I'm not sure if we'd consider this breaking or not.Impacted Spec
Which spec(s) will this pull request impact?
policy
Additional context
See #631