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

added _classes for knn classifiers #679

Merged
merged 3 commits into from
Dec 14, 2023
Merged

added _classes for knn classifiers #679

merged 3 commits into from
Dec 14, 2023

Conversation

Hk669
Copy link
Contributor

@Hk669 Hk669 commented Nov 30, 2023

Added _classes attribute for k-Nearest Neighbors (KNN) Algorithm

Description:
This pull request addresses the issue #669 and implements the addition of the _classes attribute for the k-Nearest Neighbors algorithm.

Changes Made:

Modified the KNNFit class to include the _classes attribute, providing unique classes based on the fitted model.
Updated the KNeighborsAlgorithm and KNeighborsRegressorAlgorithm classes to inherit from KNNFit and ClassifierMixin and RegressorMixin, respectively.
Adjusted relevant unit tests to ensure the proper functioning of the _classes attribute.

Testing:

  • Unit tests (test_knn.py) have been added/modified to check the functionality of the _classes attribute.
  • All tests pass successfully.

checklist:

  • Added _classes attribute.
  • Updated unit tests.
  • Checked compatibility and consistency.
tests_knn1 tests_knn classes_property

Copy link
Contributor

@pplonski pplonski left a comment

Choose a reason for hiding this comment

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

Thank you for PR. Please check how classes are derived, because now we get some strange nesting between base classes.


class KNeighborsAlgorithm(KNNFit, RegressorMixin):
class KNeighborsAlgorithm(KNNFit, ClassifierMixin):
Copy link
Contributor

Choose a reason for hiding this comment

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

this looks strange, we derive from ClassifierMixin in KNNFit as well ...

@@ -73,7 +81,7 @@ def __init__(self, params):
)


class KNeighborsRegressorAlgorithm(KNNFit):
class KNeighborsRegressorAlgorithm(KNNFit, RegressorMixin):
Copy link
Contributor

Choose a reason for hiding this comment

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

we derive from KNNFit which derives from ClassifierMixin and we also derive from RegressorMixin. This looks strange ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i derived the KNeighborsAlgorithm from both ClassifierMixin and KNNFit, where as the KNeighborsRegressorAlgorithm from both RegressorMixin and KNNFit .

KNNFit focuses on fitting the model, and KNeighborsAlgorithm and KNeighborsRegressorAlgorithm focus on the specific tasks of classification and regression, respectively.

classes = None

#debug statements
print("np.unique(self.y):", np.unique(self.y))
Copy link
Contributor

Choose a reason for hiding this comment

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

we dont need debug statements, let use asserts

@Hk669
Copy link
Contributor Author

Hk669 commented Dec 1, 2023

made the requested changes, and its working quite great.
passed all the unitttests.

  • please go throught the commits and lemme know if this works.
tests_knn_new

@Hk669
Copy link
Contributor Author

Hk669 commented Dec 4, 2023

@pplonski is this working?

@Hk669 Hk669 requested a review from pplonski December 14, 2023 13:24
@pplonski pplonski merged commit 8678d84 into mljar:master Dec 14, 2023
@pplonski
Copy link
Contributor

Thank you @Hk669 for PR, good job 🥇

@Hk669
Copy link
Contributor Author

Hk669 commented Dec 15, 2023

Thanks for the support @pplonski

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.

None yet

2 participants