-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Remove correctly the last segment in annotation #4946
Conversation
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.
Some expectations updates seem weird to me. datasources
should be per-segment?
@@ -48,10 +48,10 @@ Feature: Traffic - speeds | |||
|
|||
When I route I should get | |||
| from | to | route | speed | weights | a:datasources | | |||
| a | b | ad,de,eb,eb | 30 km/h | 1275.7,400.4,378.2,0 | 1:0:0:0 | | |||
| a | b | ad,de,eb,eb | 30 km/h | 1275.7,400.4,378.2,0 | 1:0:0 | |
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 seems wrong: a:datasources
should have the same length as weights
.
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.
@TheMarex the test case already has an inconsistency in expectation values: lines 53-54 have two weight values and two datasources, lines 55-56 have two weight values and one datasource. The change only fixes inconsistency in annotation values: now number of annotation weights, durations, data sources, speed values is by one less than number of node IDs. I'll 👀 how to make consistent step weights
and annotation a:weight
, because with master
number of weights is different for the dc
case:
| from | to | route | speed | weights | a:weight |
| a | b | ad,de,eb,eb | 30 km/h | 1275.7,400.4,378.2,0 | 1275.7:400.4:378.2:0 |
| b | c | bc,bc | 27 km/h | 741.5,0 | 741.5:0 |
| d | c | dc,dc | 36 km/h | 956.8,0 | 956.8 |
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.
Oh I see so actually the weights
are wrong in this scenario?
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 yes, #3953 is the related issue about inconsistent trimming of small segments
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 👍 on just fixing one problem at a time here.
Issue
Remove the last trimmed segment in annotations array.
/cc @freenerd @danpat
Tasklist
Requirements / Relations
Link any requirements here. Other pull requests this PR is based on?