MRG renamed ``min_n`` and ``max_n`` parameters in CountVectorizer #1024

Merged
merged 4 commits into from Aug 19, 2012

Conversation

Projects
None yet
5 participants
@amueller
Member

amueller commented Aug 13, 2012

To make grid-searching them together possible.
cc @agramfort and @ogrisel

@agramfort

This comment has been minimized.

Show comment
Hide comment
@agramfort

agramfort Aug 14, 2012

Member

+1 for merge

Member

agramfort commented Aug 14, 2012

+1 for merge

@amueller

This comment has been minimized.

Show comment
Hide comment
@amueller

amueller Aug 14, 2012

Member

Thanks for the review @agramfort :). @ogrisel as you are our NLP expert and probably know the most about this module, I'd love to have your vote, too, if you find the time.

Member

amueller commented Aug 14, 2012

Thanks for the review @agramfort :). @ogrisel as you are our NLP expert and probably know the most about this module, I'd love to have your vote, too, if you find the time.

@mblondel

This comment has been minimized.

Show comment
Hide comment
@mblondel

mblondel Aug 14, 2012

Member

Looks good to me too except that I find bound_n not terribly explicit. Maybe something like ngram_range?

Member

mblondel commented Aug 14, 2012

Looks good to me too except that I find bound_n not terribly explicit. Maybe something like ngram_range?

@amueller

This comment has been minimized.

Show comment
Hide comment
@amueller

amueller Aug 14, 2012

Member

Yeah, ngram_range sounds better. Anyone opposed?

Member

amueller commented Aug 14, 2012

Yeah, ngram_range sounds better. Anyone opposed?

@vene

This comment has been minimized.

Show comment
Hide comment
@vene

vene Aug 15, 2012

Member

+1 for ngram_range. n_range would be more semantically correct but less clear imo.

Member

vene commented Aug 15, 2012

+1 for ngram_range. n_range would be more semantically correct but less clear imo.

@amueller

This comment has been minimized.

Show comment
Hide comment
@amueller

amueller Aug 15, 2012

Member

Done. Thanks for the feedback.

Member

amueller commented Aug 15, 2012

Done. Thanks for the feedback.

@amueller

This comment has been minimized.

Show comment
Hide comment
@amueller

amueller Aug 18, 2012

Member

@ogrisel If you think this is ok, feel free to press the green button ;)

Member

amueller commented Aug 18, 2012

@ogrisel If you think this is ok, feel free to press the green button ;)

@ogrisel

View changes

doc/whats_new.rst
@@ -77,6 +77,10 @@ API changes summary
necessary for early-stopping in which case the tree is not
completely built.
+ - In :class:`feature_extraction.text.CountVectorizer` the parameters
+ ``min_n`` and ``max_n`` were joined to the parameter ``bounds_n`` to

This comment has been minimized.

@ogrisel

ogrisel Aug 18, 2012

Member

s/bounds_n/ngram_range/

@ogrisel

ogrisel Aug 18, 2012

Member

s/bounds_n/ngram_range/

+ warnings.warn('Parameters max_n and min_n are deprecated. Use '
+ 'ngram_range instead.')
+ if min_n is None:
+ min_n = 1

This comment has been minimized.

@ogrisel

ogrisel Aug 18, 2012

Member

the max_n is None case should also be handled here if I am not mistaken.

@ogrisel

ogrisel Aug 18, 2012

Member

the max_n is None case should also be handled here if I am not mistaken.

@ogrisel

This comment has been minimized.

Show comment
Hide comment
@ogrisel

ogrisel Aug 18, 2012

Member

+1 for merging once the too previous comments have been addressed. +1 for explicit parameter names in general :)

Member

ogrisel commented Aug 18, 2012

+1 for merging once the too previous comments have been addressed. +1 for explicit parameter names in general :)

@amueller amueller merged commit 4156481 into scikit-learn:master Aug 19, 2012

@amueller

This comment has been minimized.

Show comment
Hide comment
@amueller

amueller Aug 19, 2012

Member

merged by rebase.

Member

amueller commented Aug 19, 2012

merged by rebase.

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