8000 ENH Replace *args with named arguments in make_union by jaketae · Pull Request #17472 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

ENH Replace *args with named arguments in make_union #17472

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 12 commits into from
Jun 7, 2020
Merged

ENH Replace *args with named arguments in make_union #17472

merged 12 commits into from
Jun 7, 2020

Conversation

jaketae
Copy link
Contributor
@jaketae jaketae commented Jun 6, 2020 •
edited
Loading

Reference Issues/PRs

Partially addresses #17402.

What does this implement/fix? Explain your changes.

Any other comments?

This PR is part of the Data Umbrella Scikit-learn Sprint. #DataUmbrella

@jaketae jaketae changed the title test: open pr for partner fetch [WIP] Replace *args with named arguments in make_union Jun 6, 2020
@amueller
Copy link
Member
amueller commented Jun 6, 2020

This looks good, is this still work in progress?

@jaketae
Copy link
Contributor Author
jaketae commented Jun 6, 2020 •
edited
Loading

@amueller I think we're done now! The import orders were jumbled up, so I reverted them back to how they were (as per your note in the other PR). Thanks!

@jaketae jaketae changed the title [WIP] Replace *args with named arguments in make_union [MGR] Replace *args with named arguments in make_union Jun 6, 2020
@jaketae jaketae marked this pull request as ready for review June 6, 2020 17:43
@amueller
Copy link
Member
amueller commented Jun 6, 2020

Looks good, thanks! Let's wait for CI.

@amueller
Copy link
Member
amueller commented Jun 6, 2020

There's a test failure in test_make_union_kwargs

@amueller
Copy link
Member
amueller commented Jun 6, 2020

You need to change the expected error message in that test.

@jaketae
Copy link
Contributor Author
jaketae commented Jun 6, 2020

@amueller, I'm slightly confused as to how the error message should be modified. I think this is the part that needs fixing:

assert_raise_message(
        TypeError,
        'Unknown keyword arguments: "transformer_weights"',
        make_union, pca, mock, transformer_weights={'pca': 10, 'Transf': 1}
    )

I changed the error message to "make_union() got an unexpected keyword argument 'transformer_weights'". Is this the right way to go? Thanks in advance.

@amueller
Copy link
Member
amueller commented Jun 6, 2020

That's exactly right. CI is a bit slow right now, have you run the tests locally? You can do pytest path/to/the/test/file.py to run only that test.

@jaketae
Copy link
Contributor Author
jaketae commented Jun 6, 2020 •
edited
Loading

@amueller I was just doing that, and can now confirm that all tests pass on my local. Thank you!

jnothman
jnothman reviewed Jun 6, 2020
View reviewed changes
thomasjpfan
thomasjpfan reviewed Jun 7, 2020
View reviewed changes
Copy link
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.

Thank you for the PR @jaketae !

jaketae and others added 2 commits June 7, 2020 23:33
@jaketae @thomasjpfan
Update sklearn/tests/test_pipeline.py
5353df6 
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
@jaketae @thomasjpfan
Update sklearn/pipeline.py
cbb5f0d 
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
thomasjpfan
thomasjpfan approved these changes Jun 7, 2020
View reviewed changes
Copy link
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 changed the title [MGR] Replace *args with named arguments in make_union ENH Replace *args with named arguments in make_union Jun 7, 2020
@thomasjpfan thomasjpfan merged commit 6df781b into scikit-learn:master Jun 7, 2020
@jaketae jaketae deleted the make-union-args branch June 7, 2020 17:42
@alfaro96 alfaro96 mentioned this pull request Jun 16, 2020
Closed
viclafargue pushed a commit to viclafargue/scikit-learn that referenced this pull request Jun 26, 2020
@jaketae @thomasjpfan @viclafargue
ENH Replace *args with named arguments in make_union (scikit-learn#17472
1128c27 
)

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
Co-authored-by: IQRA MUHAMMAD <iqra@iqra.local>
jayzed82 pushed a commit to jayzed82/scikit-learn that referenced this pull request Oct 22, 2020
@jaketae @thomasjpfan @jayzed82
ENH Replace *args with named arguments in make_union (scikit-learn#17472
3aa449e 
)

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
Co-authored-by: IQRA MUHAMMAD <iqra@iqra.local>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jnothman jnothman jnothman left review comments

@amueller amueller amueller approved these changes

@thomasjpfan thomasjpfan thomasjpfan approved these changes

Assignees
No one assigned
Labels
module:pipeline
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jaketae @amueller @jnothman @thomasjpfan
0