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

Add segments to Flow to update traffic on part of way #322

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

serho
Copy link

@serho serho commented May 4, 2020

Issue

Description

A summary description for what the PR achieves.

  • Add segments to Flow to update traffic only on part of way.

Tasklist

Prerequirements

  • Want to contribute? Great! First, please read this page Contribution Guidelines.
  • If your PR is still work in progress please attach the relevant label.

@serho serho force-pushed the feature/add-segmented-flow branch from 3eb418d to ae0438a Compare May 4, 2020 17:33
@@ -109,6 +128,17 @@ func task(wayid2speed map[int64]int, source <-chan string, sink chan<- string, d
tasksWg.Done()
}

func getSpeedOfSegments(segments []*trafficproxy.SegmentedFlow, speeds []int, nodesCnt uint64) {
Copy link

@CodeBear801 CodeBear801 May 5, 2020

Choose a reason for hiding this comment

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

  1. Would be better to have some protection at beginning of for _, segment := range segments {:
0 <= segment.Begin < segment.End <=100

Ignore unexpected data and generate a warning.

  1. How about following situation:
    A long edge just has two nodes, and we generate a traffic flow segment
[speed:25 begin:25 end:75 ]

It will result

node_1, node_1, 25

This information won't be generated which is correct from code. getSpeedOfSegments skips adjustment for speeds and will keep original value from speedsFwd /speedsBwd.
But in reality, when we adjust speed for part of segment, we don't know how the nodes is organized. May be better to log such situation into statistics in case speed data is dropped unexpectedly?

Copy link
Author

Choose a reason for hiding this comment

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

@CodeBear801 I've added the warning and info into statistics

Copy link

@wangyoucao577 wangyoucao577 left a comment

Choose a reason for hiding this comment

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

@serho It looks good. Have your tried it in your application already? How many segmented flows added compare with original way based flows(E.g., 50% more)? Does it significantly affect on performance?

integration/cmd/osrm-traffic-updater/speed_table_dumper.go Outdated Show resolved Hide resolved
@@ -62,6 +89,17 @@ func loadMockTrafficFlow2Map(wayid2speed map[int64]int) {
wayid2speed[-24418344] = 59
}

func loadMockTrafficFlowSegment2Map(segmentsOfWay map[int64][]*trafficproxy.SegmentedFlow) {
segmentsOfWay[733690162] = []*trafficproxy.SegmentedFlow{
{Speed: 25, Begin: 25, End: 75},

Choose a reason for hiding this comment

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

It tests part of way have segmented flow. I'm thinking about the case that every segments of way have segmented flow. E.g., {Speed: 10, Begin: 0, End: 24},{Speed: 25, Begin: 25, End: 75},{Speed: 60, Begin: 76, End: 100}. Is it possible in your application?

Copy link
Author

Choose a reason for hiding this comment

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

It's possible. I've added the test case.

CHANGELOG-FORK.md Outdated Show resolved Hide resolved
@serho
Copy link
Author

serho commented May 5, 2020

@wangyoucao577

Have your tried it in your application already?

I'm working on the integration with our app. I've checked it manually via mali.js.org

How many segmented flows added compare with original way based flows(E.g., 50% more)? Does it significantly affect on performance?

At this moment we have a not big osm.pbf file and also there aren't a lot of speed limits in our app to check performance. But when the integration is finished I'd be able to generate a batch of Flows with and without segments to compare.

@serho serho force-pushed the feature/add-segmented-flow branch from 503cb9d to 23a7824 Compare May 5, 2020 14:43
Copy link

@wangyoucao577 wangyoucao577 left a comment

Choose a reason for hiding this comment

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

@wangyoucao577

Have your tried it in your application already?

I'm working on the integration with our app. I've checked it manually via mali.js.org

How many segmented flows added compare with original way based flows(E.g., 50% more)? Does it significantly affect on performance?

At this moment we have a not big osm.pbf file and also there aren't a lot of speed limits in our app to check performance. But when the integration is finished I'd be able to generate a batch of Flows with and without segments to compare.

OK. Do you mind that we merge the codes after you finish the integration in your application? You may possible to show us some log to make sure it works well. Recently we'll not have such data to test it in real situation, so your integration will be the most important verifcation after unit test.

}

indexOfBegin := int(math.Floor(float64(nodesCnt) * float64(segment.Begin) / 100))
indexOfEnd := int(math.Floor(float64(nodesCnt) * float64(segment.End) / 100))

Choose a reason for hiding this comment

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

Here might be some problem due to floating calculation. E.g., totally 11 nodes and 3 segmented flows {Speed: 10, Begin: 0, End: 27},{Speed: 25, Begin: 28, End: 75},{Speed: 60, Begin: 76, End: 100} on a way, and the segmented flows will be applied to node 0~2, 3~8, 8~11, but the 2~3 is missed.
To solve the problem, it might be better to sort the segmented flows of the way first, then adjust the nodes offset, i.e. 3~8 -> 2~8.

Copy link
Author

Choose a reason for hiding this comment

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

@wangyoucao577 I got it. But maybe it is ok.

For example, if first End and second Begin have the same osm nodeID, the segments will be {Speed: 10, Begin: 0, End: 27}, {Speed: 20, Begin: 27, End: 75}. It works for the same dataset on each side (traffic-updater and traffic-proxy).

On the other hand, I could add this condition

if lastEnd - curBegin == 1 {
	indexOfBegin + 1
}

But it will affect all segments i.e 8~11 -> 7~11

May be

if lastEnd - curBegin == 1 && lastIndexOfEnd != indexOfBegin {
	indexOfBegin + 1
}

Choose a reason for hiding this comment

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

Oh! Maybe this issue will gone if we simply to make sure next begin == last end. I.e.,
first segment: {Speed: 10, Begin: 0, End: 27}, then {Speed: 25, Begin: 27, End: 75} instead of {Speed: 25, Begin: 28, End: 75} as second segment(Begin: 28 ->Begin: 27).
How's your opinion?

@serho
Copy link
Author

serho commented May 6, 2020

@wangyoucao577

Do you mind that we merge the codes after you finish the integration in your application? You may possible to show us some log to make sure it works well. Recently we'll not have such data to test it in real situation, so your integration will be the most important verifcation after unit test.

Yes, I do. It's a good point.

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.

How can I update the specific part of the way?
3 participants