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

Hierarchical clustering? #4

Closed
dreiss-isb opened this issue May 14, 2013 · 12 comments
Closed

Hierarchical clustering? #4

dreiss-isb opened this issue May 14, 2013 · 12 comments

Comments

@dreiss-isb
Copy link

Any plans / interest to add hierarchical clustering to this package? Or is that more appropriate for a different package? In that case, this package should be renamed to Kmeans or some such.

@johnmyleswhite
Copy link
Member

Yes, I'd like to have hierarchical clustering in this package. I'd love a pull request, but will otherwise get to it over the course of the summer.

@MatthewRalston
Copy link

Definitely would love to see this!

@lindahua
Copy link
Contributor

lindahua commented Nov 6, 2014

There is a PR #32 on this, which is being under review.

@diegozea
Copy link

diegozea commented Mar 8, 2016

There is already a function for hierarchical clustering, but the API doesn't follow the style of this package. Is It possible to...

  • ...make Hclust a sub-type of ClusteringResult?
  • ...rename cutree to assignments (with h and k keyword arguments)?
  • ...define counts with h and k keyword arguments?
  • ...define nclusters with h keyword argument?

Also, the name hierarchical sounds more Julian than hclust.

If it's ok, I could make a PR after #59 (and maybe before #60) get merged.

What do you think?

Thanks!

Best

@diegozea
Copy link

diegozea commented May 6, 2016

Bump!

@kmsquire
Copy link
Contributor

kmsquire commented May 7, 2016

I think that would be fine. Do you feel comfortable reviewing #59 and #60? I think they could probably be merged if someone reviewed them and gave some feedback (even a "looks good").

@diegozea
Copy link

diegozea commented May 7, 2016

After viewing the changes and comparing results with R, #59 looks good ;)

@ararslan
Copy link
Member

Now that we have HClust in here, I'll close this as having been addressed.

@diegozea
Copy link

@ararslan Was there a recent merged PR addressing the documentation and API of HClust to close this?

@ararslan
Copy link
Member

@diegozea Heirarchical clustering was introduced in #32. There's still an outstanding PR (#60) to add documentation, but since this issue is asking for heirarchical clustering functionality and that's now available, it seems to me that the issue has been effectively addressed.

@diegozea
Copy link

diegozea commented Jul 23, 2016

@ararslan What do you think about this proporsal: #4 (comment) ?
I'm sorry. I haven't had time to work on it yet...

@ararslan
Copy link
Member

What you propose in that comment sounds great to me!

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

No branches or pull requests

7 participants