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

Hcluster v2 #86

Merged
merged 11 commits into from Apr 1, 2011

Conversation

Projects
None yet
3 participants
@agramfort
Member

agramfort commented Feb 22, 2011

Hi folks,

I've update the current hierarchial clustering with the changes in master.

A quick update on the status (I hope I did not forget anything from previous comments):

done

  • pipeline and GridSearchCV have been cleaned
  • feat agglo inherits from TransformerMixin
  • s / n_comp / n_components

todo

  • s / memory / cache ?
  • check cython inertia necessary
  • scipy comparison and auto select best implementation
  • rst doc for feature agglomeration. Where should it appear?
@GaelVaroquaux

This comment has been minimized.

Show comment
Hide comment
@GaelVaroquaux

GaelVaroquaux Feb 22, 2011

Member

Your list looks good. I would merge with that :).

With regards to where feature agglomeration should go, one option is in the clustering documentation.

Member

GaelVaroquaux commented Feb 22, 2011

Your list looks good. I would merge with that :).

With regards to where feature agglomeration should go, one option is in the clustering documentation.

@GaelVaroquaux

This comment has been minimized.

Show comment
Hide comment
@GaelVaroquaux

GaelVaroquaux Feb 26, 2011

What was the bug? You are introducing a new function, but not modifying old one. It scares me a bit to read a commit message like your's for a commit that is only introducing functionality.

GaelVaroquaux commented on 5c9df2c Feb 26, 2011

What was the bug? You are introducing a new function, but not modifying old one. It scares me a bit to read a commit message like your's for a commit that is only introducing functionality.

This comment has been minimized.

Show comment
Hide comment
@agramfort

agramfort Feb 26, 2011

Owner

well I think there was no bug in the end ... but there is something I need to clarify... I'll probably revert this commit

Owner

agramfort replied Feb 26, 2011

well I think there was no bug in the end ... but there is something I need to clarify... I'll probably revert this commit

ENH : factorizing img_to_graph and grid_to_graph
there was no bug before but an obscure img_to_graph(mask, mask)
that is now replaced by grid_to_graph (makes a connectivity structure
for a grid of pixels/voxels)
@GaelVaroquaux

This comment has been minimized.

Show comment
Hide comment
@GaelVaroquaux

GaelVaroquaux Feb 28, 2011

Ones on the diagonal, please, or you will have many graph algorithms bugging.

Also, you should probably make sure that the dtype is the specified one.

Ones on the diagonal, please, or you will have many graph algorithms bugging.

Also, you should probably make sure that the dtype is the specified one.

@GaelVaroquaux

This comment has been minimized.

Show comment
Hide comment
@GaelVaroquaux

GaelVaroquaux Feb 28, 2011

Ones on the diagonal, please, or you will have many graph algorithms bugging.

Also, you should probably make sure that the dtype is the specified one.

Finally, the lines above have too many if/else. They could be simplified with a unique if/else which would make the code more readable.

Ones on the diagonal, please, or you will have many graph algorithms bugging.

Also, you should probably make sure that the dtype is the specified one.

Finally, the lines above have too many if/else. They could be simplified with a unique if/else which would make the code more readable.

@GaelVaroquaux

This comment has been minimized.

Show comment
Hide comment
@GaelVaroquaux

GaelVaroquaux commented on 06fffc7 Feb 28, 2011

Merki!

@bthirion

This comment has been minimized.

Show comment
Hide comment
@bthirion

bthirion Apr 1, 2011

Member

I've updated it on my github: systematically using scipy when the connectivity is None in ward's algo.
(NB : I can't push to your github ?)
We should be able to merge ?

Bertrand

Member

bthirion commented Apr 1, 2011

I've updated it on my github: systematically using scipy when the connectivity is None in ward's algo.
(NB : I can't push to your github ?)
We should be able to merge ?

Bertrand

@GaelVaroquaux

This comment has been minimized.

Show comment
Hide comment
@GaelVaroquaux

GaelVaroquaux Apr 1, 2011

Member

What's the test coverage like? If it's good, I suggest you merge.

Member

GaelVaroquaux commented Apr 1, 2011

What's the test coverage like? If it's good, I suggest you merge.

@bthirion

This comment has been minimized.

Show comment
Hide comment
@bthirion

bthirion Apr 1, 2011

Member

"""
we should get rid of the old ward and non standard method.
I don't like this inertia_criterion=True | False
"""
done + pep8 ok
on my branch: https://github.com/bthirion/scikit-learn/tree/hcluster2

Member

bthirion commented Apr 1, 2011

"""
we should get rid of the old ward and non standard method.
I don't like this inertia_criterion=True | False
"""
done + pep8 ok
on my branch: https://github.com/bthirion/scikit-learn/tree/hcluster2

@agramfort

This comment has been minimized.

Show comment
Hide comment
@agramfort

agramfort Apr 1, 2011

Member

just pushed your commit here. I vote for merging. ok?

Member

agramfort commented Apr 1, 2011

just pushed your commit here. I vote for merging. ok?

@GaelVaroquaux

This comment has been minimized.

Show comment
Hide comment
@GaelVaroquaux

GaelVaroquaux Apr 1, 2011

Member

+1 for merge!

Member

GaelVaroquaux commented Apr 1, 2011

+1 for merge!

@agramfort agramfort merged commit 46ce94b into scikit-learn:master Apr 1, 2011

@agramfort

This comment has been minimized.

Show comment
Hide comment
@agramfort

agramfort Apr 1, 2011

Member

merged

Member

agramfort commented Apr 1, 2011

merged

@GaelVaroquaux

This comment has been minimized.

Show comment
Hide comment
@GaelVaroquaux

GaelVaroquaux Apr 1, 2011

Member

F*in' Awesome!

ADIDAS (All Day I Dream About Scikit-learn).

Member

GaelVaroquaux commented Apr 1, 2011

F*in' Awesome!

ADIDAS (All Day I Dream About Scikit-learn).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment