-
Notifications
You must be signed in to change notification settings - Fork 208
[ENH] Fixes Issue Improve _check_params
method in kmeans.py
and kmedoids.py
#2682
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
base: main
Are you sure you want to change the base?
Conversation
Thank you for contributing to
|
_check_params
method in kmeans.py
and kmedoids.py
_check_params
method in kmeans.py
and kmedoids.py
aeon/clustering/_k_medoids.py
Outdated
def _incorrect_params(self) -> str: | ||
return ( | ||
f"The value provided for init: {self.init} is " | ||
f"invalid. The following are a list of valid init algorithms " | ||
f"strings: random, kmedoids++, first, build. You can also pass a " | ||
f"np.ndarray of size (n_clusters, n_channels, n_timepoints)" | ||
) |
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.
dont do this, just has a variable with the string if needed to avoid repetition.
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.
got it will make the changes
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.
dont do this, just has a variable with the string if needed to avoid repetition.
I have pushed the changes, though I am not sure why I am getting the errors for some of the github actions. The only change I have made is adding some strings.
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 do not see a variable used? the message just appears to be duplicated now.
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 do not see a variable used? the message just appears to be duplicated now.
Will add
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 do not see a variable used? the message just appears to be duplicated now.
I have made the changes
499ba8b
to
edea320
Compare
@MatthewMiddlehurst I wasn't sure if there is any other ways to deal with this other than rebasing and I did this to move my latest commit Now the tests are passing. I ran the following commands:
|
I do not know, i cannot see an error any more due to the force push. |
I was just wondering if force pushing is fine in this case? Or would that be a problem? |
I do not know enough about the issue or why it happens, not looked into it. See #2723 |
first k time series as centroids. Build [1] greedily selects the k medoids | ||
by first selecting the medoid that minimizes the sum of distances | ||
to all other points(this point is the most centrally located) and then | ||
iteratively selects the next k-1 medoids that maximizes the decrease in sum | ||
of distances of all other points to their respective medoids selected so far. |
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.
Is this the original method used in the paper?
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.
Reference Issues/PRs
Fixes: #2681
What does this implement/fix? Explain your changes.
Adds an else statement in
_check_params
for bothkmeans.py
andkmedoids.py
. Also adds build initialization documentation.Does your contribution introduce a new dependency? If yes, which one?
Any other comments?
PR checklist
For all contributions
For new estimators and functions
__maintainer__
at the top of relevant files and want to be contacted regarding its maintenance. Unmaintained files may be removed. This is for the full file, and you should not add yourself if you are just making minor changes or do not want to help maintain its contents.For developers with write access