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

Adding KNNClassificationDecider #463

Open
wants to merge 143 commits into
base: KNNDecider
Choose a base branch
from

Conversation

david-z-shi
Copy link
Contributor

Reference issue

Closes #461.

Type of change

Adding KNNClassificationDeciders.

What does this implement/fix?

Adding experiments for KNNClassificationDecider and the associated deciders for these experiments

Additional information

Experiments need more compute and reps.

david-z-shi and others added 30 commits September 18, 2020 17:33
This reverts commit f48f6c2, reversing
changes made to aa3ecf1.
updated with main repo 10/27/2020
Co-Authored-By: jmandav1 <39231283+jmandavilli@users.noreply.github.com>
Co-Authored-By: parthgvora <parthgvora@gmail.com>
Co-Authored-By: ypeng22 <68977380+ypeng22@users.noreply.github.com>
…ecision_boundaries_functions.py

Co-Authored-By: jmandav1 <39231283+jmandavilli@users.noreply.github.com>
Co-Authored-By: parthgvora <parthgvora@gmail.com>
Co-Authored-By: jmandav1 <39231283+jmandavilli@users.noreply.github.com>
Co-Authored-By: parthgvora <parthgvora@gmail.com>
Indexing into a list using an array of integers does not work and for some reason in my use case that's what self.classes is. This simply makes the array-ness of self.classes explicit.
@david-z-shi
Copy link
Contributor Author

@jdey4 Let me know if I need to make any other changes.

@PSSF23 PSSF23 requested a review from jdey4 May 3, 2021 20:11
Copy link
Member

@PSSF23 PSSF23 left a comment

Choose a reason for hiding this comment

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

I see that the KNN branch is pretty outdated, so I would suggest not closing your issue by this PR. @jdey4 Perhaps we can merge this one and then merge KNNDecider to staging? Or should the branches stay separated?

Copy link
Member

@PSSF23 PSSF23 left a comment

Choose a reason for hiding this comment

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

@latasianguy And I see that the black check failed. Check the log and format your code~

Copy link
Member

@PSSF23 PSSF23 left a comment

Choose a reason for hiding this comment

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

@latasianguy It's too hard to locate where you exactly changed. Could you make a list of any "new" files you added in this PR?

@david-z-shi
Copy link
Contributor Author

Yep, everything here and here is new, and there are additional transformers, voters, and deciders in here.

@PSSF23 PSSF23 requested review from sampan501 and eigenvivek May 6, 2021 00:04
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.

10 participants