Skip to content

Removed redundant parameters of 2 classes#14796

Merged
thomasjpfan merged 3 commits intoscikit-learn:masterfrom
RuchitaGarde:redundant
Sep 10, 2019
Merged

Removed redundant parameters of 2 classes#14796
thomasjpfan merged 3 commits intoscikit-learn:masterfrom
RuchitaGarde:redundant

Conversation

@RuchitaGarde
Copy link
Copy Markdown
Contributor

Reference Issues/PRs

Fixes #14351

What does this implement/fix? Explain your changes.

Removed redundant parameters from 2 classes:
make_blobs() - 2 instances
make_classification() - 1 instance

Any other comments?

Working with @ttang131

Copy link
Copy Markdown
Member

@rth rth left a comment

Choose a reason for hiding this comment

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

PEP8 formatting issues, otherwise LGTM. Thanks!

# Generate data
X, y = make_blobs(n_samples=1000, n_features=2, random_state=42,
X, y = make_blobs(n_samples=1000, random_state=42,
cluster_std=5.0)
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.

This can be put on 1 line now.

X, y = datasets.make_classification(n_samples=100000, n_features=20,
n_informative=2, n_redundant=10,
X, y = datasets.make_classification(n_samples=100000,
n_redundant=10,
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.

This is not PEP8 compliant (and also all of it could fit on a single line).

@NicolasHug
Copy link
Copy Markdown
Member

Thanks for the PR @RuchitaGarde , can you please address the comments? Thanks!

Copy link
Copy Markdown
Member

@NicolasHug NicolasHug left a comment

Choose a reason for hiding this comment

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

I've addressed the indentation comments, LGTM

@reshamas
Copy link
Copy Markdown
Member

reshamas commented Sep 9, 2019

@kellycarmody 
Can you drop Ruchita an email?
Please give 3 days for response.  If no response we can either complete this or return it to issue pool.  
Thanks.

@RuchitaGarde
Copy link
Copy Markdown
Contributor Author

I will look into this.

@NicolasHug
Copy link
Copy Markdown
Member

@RuchitaGarde I've addressed the comments already so no worries. Maybe other reviewers will require more changes, in which case we'll ping you again

Copy link
Copy Markdown
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

LGTM

@thomasjpfan thomasjpfan merged commit 266bfe8 into scikit-learn:master Sep 10, 2019
@reshamas
Copy link
Copy Markdown
Member

@RuchitaGarde @ttang131
Thanks for joining us at the #wimlds sprint!

If you haven't yet, please complete the sprint feedback survey.

cc: @kellycarmody

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.

Remove redundant parameter assignment in examples/tests

5 participants