Skip to content

[MRG + 1] fixed comment in 20newsgroups example#7423

Merged
jnothman merged 1 commit intoscikit-learn:masterfrom
kmike:20newsgroups
Oct 25, 2016
Merged

[MRG + 1] fixed comment in 20newsgroups example#7423
jnothman merged 1 commit intoscikit-learn:masterfrom
kmike:20newsgroups

Conversation

@kmike
Copy link
Copy Markdown
Contributor

@kmike kmike commented Sep 14, 2016

Here:

data_train = fetch_20newsgroups(subset='train', categories=categories,
                                shuffle=True, random_state=42,
                                remove=remove)

data_train.target_names must be used as category names even if categories is not None because the order can be different from categories order.

Copy link
Copy Markdown
Contributor

@nelson-liu nelson-liu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense, LGTM

@nelson-liu
Copy link
Copy Markdown
Contributor

maybe have to rerun ci (i guess you can do this manually by simply pushing a spurious change and removing it) due to b1bd976

print('data loaded')

categories = data_train.target_names # for case categories == None
categories = data_train.target_names # order may be different
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry but I find comment obscure

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I agree. What do you think about the updated comment?

data_train.target_names must be used even if
categories is not None because target_names order can
be different from 'categories' order.
@kmike
Copy link
Copy Markdown
Contributor Author

kmike commented Oct 24, 2016

ping :) Will the PR be more mergeable if I just remove "order of labels in target_names can be different from categories" comment?

@amueller
Copy link
Copy Markdown
Member

lgtm

@amueller amueller changed the title fixed comment in 20newsgroups example [MRG + 1] fixed comment in 20newsgroups example Oct 24, 2016
@jnothman jnothman merged commit 6b381ae into scikit-learn:master Oct 25, 2016
@jnothman
Copy link
Copy Markdown
Member

thanks

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.

5 participants