-
Notifications
You must be signed in to change notification settings - Fork 156
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
Sort the mergeable segments before computing merged segments #1207
Merged
Merged
Changes from 10 commits
Commits
Show all changes
22 commits
Select commit
Hold shift + click to select a range
545344b
add long input test case
isVoid cbd803f
sort the segments before merging
isVoid 04d551e
add twospaces find and combine test
isVoid bbc2dce
add twospaces, non-contiguous input
isVoid 83d19af
Remove stale includes
isVoid 4a822e5
add documentation
isVoid 19fc646
add test for overlapping and non-contiguous inputs
isVoid f000007
Remove stale debug prints
isVoid 3ec5634
Update documentation
isVoid 8713c49
remove large intersection test
isVoid bac1a36
applied review comments
isVoid 03b8338
Add streams to allocate_like call
isVoid 571318c
Add streams to allocate_like call
isVoid 76d066e
address review changes
isVoid 6d0ae8b
Merge branch 'branch-23.08' of https://github.com/rapidsai/cuspatial …
isVoid a3fc039
Merge branch 'fix/allocate_like' into fix/1200
isVoid 5478f37
fix collinear test bug
isVoid 37d3808
style
isVoid 5f3f9e1
Add comments to explain algorithm complexity
isVoid c860c57
Merge branch 'branch-23.08' into fix/1200
isVoid 6f13294
Change test case result order due to sorting
isVoid 6b82b34
Merge branch 'fix/1200' of github.com:isVoid/cuspatial into fix/1200
isVoid File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Perhaps naive is the wrong word. "brute force"? Is lower complexity a possibility?
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.
Great question. What's the lower bound of the algorithm complexity here? Given a set of segments, to merge all mergeable segments, one at least needs to look at all segments once, so the lower bound is
O(N)
(N being the number of segments in the set). To achieve that, I assume you need to design some sort of hashing function. When you look at one segment, the hashing function will compute the key and look into the map to find if there is existing segment that can be merged with it. This key would be similar to what we designed in the sorting comparator here - a combination of group id, slope and lower left of the segment. A map on CPU code is simple, but may be quite difficult to implement on device and could invoke unwanted slow down.So that said, the current algorithm isn't naive (or brute force) at all - while there is a nested loop here, the sorting actually already did part of the hashing work like above. The sorting makes sure that the outer loop only run against all other segments once in every mergeable group, all subsequent segments are marked
merged_flag == 1
after the initial run. So the nested loop here is actually on the order of O(N) here. The aggregated complexity is dominated by the sorting, which is O(NlogN).