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] Refactor DBSCAN to use ml-prims. #44

Merged
merged 59 commits into from
Dec 22, 2018

Conversation

cjnolet
Copy link
Member

@cjnolet cjnolet commented Dec 2, 2018

This merge request encompasses a very large body of work and I have squashed the commits to make it easier for the reviewers.

The tests all pass, however, the DBSCAN Jupyter notebook seems to indicate there is a bug that could be causing a scalability issue.

The following error prints when I attempt to run the cuml DBSCAN training on 1000 rows of the mortgage dataset:

terminate called after throwing an instance of 'thrust::system::system_error'
  what():  scan failed to synchronize: an illegal instruction was encountered

@dantegd dantegd changed the title Refactor DBSCAN to use ml-prims. [WIP] Refactor DBSCAN to use ml-prims. Dec 2, 2018
@dantegd dantegd added the 2 - In Progress Currenty a work in progress label Dec 2, 2018
@dantegd
Copy link
Member

dantegd commented Dec 2, 2018

I tagged it as in progress while we look into the scalability issue, I’ll also try running your branch on Monday :)

@dantegd
Copy link
Member

dantegd commented Dec 3, 2018

@cjnolet Any chance you could provide a summary of what the cleanup (excluding dbscan) encompasses so that we can include it in the change log in #43

@cjnolet
Copy link
Member Author

cjnolet commented Dec 3, 2018

Sure, I've incorporated the latest changes to the ml-prims code, including the new distance API. Also removed unused code and merged the separate dbscan run paths for the Python & Googletests API.

@mike-wendt mike-wendt changed the base branch from master to branch-0.5 December 13, 2018 23:40
@cjnolet cjnolet changed the title [WIP] Refactor DBSCAN to use ml-prims. [REVIEW] Refactor DBSCAN to use ml-prims. Dec 18, 2018
@cjnolet
Copy link
Member Author

cjnolet commented Dec 18, 2018

This is ready to be reviewed & merged.

@cjnolet cjnolet added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currenty a work in progress labels Dec 18, 2018
@dantegd
Copy link
Member

dantegd commented Dec 19, 2018

This PR means that PR #47 is no longer needed and we can just close it, right?

@cjnolet
Copy link
Member Author

cjnolet commented Dec 19, 2018 via email

@mike-wendt mike-wendt added this to PR-WIP in v0.5 Release via automation Dec 20, 2018
v0.5 Release automation moved this from PR-WIP to PR-Reviewer approved Dec 21, 2018
Copy link
Member

@dantegd dantegd left a comment

Choose a reason for hiding this comment

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

PR is good, pending running tests on CUDA 10

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
No open projects
v0.5 Release
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants