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

add metric argument to allow for other linkages #222

Merged
merged 6 commits into from
Mar 6, 2023

Conversation

mikhailsirenko
Copy link
Contributor

No description provided.

@mikhailsirenko
Copy link
Contributor Author

A minor tweak to allow for the ward linkage in clusterer.apply_agglomerative_clustering

@quaquel quaquel self-requested a review March 3, 2023 13:30
Copy link
Owner

@quaquel quaquel left a comment

Choose a reason for hiding this comment

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

Changes are fine, but can you add NumPy doc style documentation?

@mikhailsirenko
Copy link
Contributor Author

Changes are fine, but can you add NumPy doc style documentation?

Will that work? Or it is too extensive and you prefer to have it as:

metric : str

@quaquel
Copy link
Owner

quaquel commented Mar 5, 2023

documentation looks fine this way. Thanks.

@EwoutH
Copy link
Collaborator

EwoutH commented Mar 5, 2023

Thanks, feature and docs look good to me!

Do we want to add tests and/or an example (basically why and when p would us this)?

@quaquel
Copy link
Owner

quaquel commented Mar 5, 2023

There is a flu_timeseries_cluster.py example for the clustering in general. I don't think we need an additional example. The reference to the scipy documentation should suffice. Clusterer itself is 100% tested, but we currently don't explicitly test for the metrics kwarg. I can see arguments for and against adding test for this. For now, I am fine with merging this.

@quaquel quaquel self-requested a review March 6, 2023 23:54
@quaquel quaquel merged commit f3e10d6 into quaquel:master Mar 6, 2023
@EwoutH
Copy link
Collaborator

EwoutH commented Mar 7, 2023

Thanks @mikhailsirenko!

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.

3 participants