-
Notifications
You must be signed in to change notification settings - Fork 3
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
Gpushift dev #1
base: master
Are you sure you want to change the base?
Gpushift dev #1
Conversation
boykovdn
commented
Feb 17, 2021
- Basic clustering step
- sklearn-like interface
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.
Thanks a lot for the PR Boyko! I've added some minor comments, it would be great if you could have a look before merging :)
|
||
def predict(self, X): | ||
r""" | ||
Predict cluster belonging based on which cluster center is the closest. |
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.
belonging -> assignment
gpushift/sklearn/meanshift.py
Outdated
# spherical = self._get_distance_metric('spherical') | ||
# | ||
# composite = lambda x,y : euclidean(x,y)**2 + spherical(x,y) | ||
# |
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.
If this block is no longer needed (as you have implemented it differently above), I would prefer to delete it. Same for the other commented blocks above.
gpushift/sklearn/meanshift.py
Outdated
|
||
self.cluster_centers_ = None | ||
|
||
self.meanshift_step = MeanShiftStep(bandwidth=bandwidth, kernel=kernel, use_keops=use_keops) |
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.
Was it your intention to not pass the distance metric here? I thought not only the final clustering but also every MS step should use the chosen metric.
:param distance_metric: callable or None | ||
If None, uses standard Euclidean distances. For special applications, | ||
the passing of a custom distance function is allowed. |
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.
I think it would be good to add the required signature of distance metric
if it is a callable, including the shapes of the input tensors, to the docstring.
Reminder to also add an example using the new interface (I will do that) |