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

[REVIEW] construct quadtree on large-scale point data #143

Merged
merged 62 commits into from
May 19, 2020

Conversation

zhangjianting
Copy link
Contributor

@zhangjianting zhangjianting commented Mar 26, 2020

Implementation of an efficient bottom-up approach to constructing quadtree on large-scale point data.

@zhangjianting zhangjianting requested review from a team as code owners March 26, 2020 02:10
@zhangjianting zhangjianting added the 3 - Ready for Review Ready for review by team label Mar 26, 2020
@zhangjianting zhangjianting changed the title construct quadtree on large-scale point data [REVIEW] construct quadtree on large-scale point data Mar 26, 2020
@zhangjianting
Copy link
Contributor Author

rerun test

@zhangjianting
Copy link
Contributor Author

rerun tests

@mike-wendt
Copy link
Contributor

mike-wendt commented Mar 26, 2020

@zhangjianting I see the issues, I'm looking at them

I've replayed the failed job and monitoring... jobs passed, running again to get clean ✅ across all jobs

@mike-wendt
Copy link
Contributor

rerun tests

Copy link
Contributor

@cwharris cwharris left a comment

Choose a reason for hiding this comment

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

Partial review. It's been sitting here for a couple of days and I haven't gotten back to it yet. Sorry. :(

Copy link
Contributor

@codereport codereport left a comment

Choose a reason for hiding this comment

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

Found a couple things before I realized this is in cuspatial and only showed up in my notifications b/c @cwharris @-ed me XD

Copy link
Member

@harrism harrism left a comment

Choose a reason for hiding this comment

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

Partial review. Just now getting to the meat of it...

Copy link
Member

@harrism harrism left a comment

Choose a reason for hiding this comment

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

I think I am about half done. Will continue tomorrow.

quad_point_count.begin(), quad_child_count.begin(),
stream);

// Shrink vectors' underlying device allocations to reduce peak memory usage
Copy link
Member

Choose a reason for hiding this comment

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

Future optimization: don't bother shrinking if the data size is under a threshold (perhaps an option).

Copy link
Member

@harrism harrism left a comment

Choose a reason for hiding this comment

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

OK, last review. Mostly just more brief docs.

@trxcllnt
Copy link
Contributor

rerun tests

@trxcllnt
Copy link
Contributor

This is failing at the moment because cudf is installed from last night, but ci/gpu/build.sh is using the headers from today. Since rapidsai/cudf#4972 was merged, the test utils from today are referencing a list_view type that isn't in the cudf/types.hpp from last night.

Copy link
Member

@harrism harrism left a comment

Choose a reason for hiding this comment

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

Approve, but please run spellcheck.

@trxcllnt trxcllnt merged commit ab95ad4 into rapidsai:branch-0.14 May 19, 2020
@zhangjianting zhangjianting deleted the fea-point-quadtree branch October 28, 2020 03:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants