Skip to content

add metric argument to allow for other linkages#222

Merged
quaquel merged 6 commits intoquaquel:masterfrom
mikhailsirenko:feature-clusterer-add-metric
Mar 6, 2023
Merged

add metric argument to allow for other linkages#222
quaquel merged 6 commits intoquaquel:masterfrom
mikhailsirenko:feature-clusterer-add-metric

Conversation

@mikhailsirenko
Copy link
Copy Markdown
Contributor

No description provided.

@mikhailsirenko
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Owner

quaquel commented Mar 5, 2023

documentation looks fine this way. Thanks.

@EwoutH
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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