agramfort/scikit-learn — comment on commit 367cd4ad150f493a4f95ccc87a86d9cabf1868c5
File:  line 0
By agramfort:

> Hi, sorry for the late response, I was quite busy yesterday and did not find the time to look at your code in detail.

no pb.

> Some comments:
> a) For implementing a new linkage criterion, one has to implement in your design a setup function (like _ward_setup) and a callable (like _Ward). This corresponds to the __init__ method (for setting things up) and the update method (corresponding to the callable). I can go with your design; I am just curious why you guys prefer this approach?

we're usually reluctant to have new objects that later need to be documented.
With a heap ie. a native data structure there is no documentation needed and
and its straightforward to explain that the callable should act on a heap to
update when merging. It's makes life easier in the long term.

> b) What dou you think about renaming the variables "A", "coord_row", and "coord_col"? I don't think the names are very intuitive. I have also the impression that there is some redundancy between A and "coord_row"/"coord_col". Maybe there would be an other data structure that could unify these three variables. Have to think about it...

+1 the hate these names.

> c) Your design: data representation by named tuple, several module level function that act on it. My design: data representation in class and methods that provide functionality. I'm personally more inclined to the object-oriented design (as you might have guessed), but we can also go with yours. Just for understanding: Why do you prefer it? Just for efficiency? Is there really such a big difference?

same as point 1. namedtuples are standard. If we need methods that act
on the dendogram I'd rather have them in HierarchicalClustering object
that respects the estimator interface.

thanks for taking the time to review my suggestions !