Skip to content

[MRG + 1] Use immutable default arguments throughtout repo #4409

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

Merged
merged 1 commit into from
Mar 20, 2015

Conversation

bryandeng
Copy link
Contributor

Hi developers:

I've now fixed the issue that some functions are using mutable default arguments, as shown in https://landscape.io/github/scikit-learn/scikit-learn/76/messages/error
(not including the one in ridge.py which has been fixed by DonBeo).

@amueller
Copy link
Member

Thanks LGTM. +1 if travis is green.

@amueller
Copy link
Member

If you squash them they will be easier to cherry pick.

@bryandeng bryandeng force-pushed the immutable_defaults branch from c2934f1 to 956f88a Compare March 18, 2015 20:42
@bryandeng
Copy link
Contributor Author

Now the four commits have been squashed into one.

@amueller
Copy link
Member

thanks :)

@landscape-bot
Copy link

Code Health
Repository health decreased by 0.04% when pulling 956f88a on bryandeng:immutable_defaults into c4df19d on scikit-learn:master.

@landscape-bot
Copy link

Code Health
Repository health decreased by 0.04% when pulling 956f88a on bryandeng:immutable_defaults into c4df19d on scikit-learn:master.

@amueller
Copy link
Member

@carlio again, not sure what is happening here :-/

@amueller
Copy link
Member

Travis failed.

@bryandeng
Copy link
Contributor Author

Travis shows that problems are in my modified rfe.py. It's not compatible with set_params().

TypeError: set_params() argument after ** must be a mapping, not NoneType

I think I can't simply modify init that way.
I'll do local tests before the next commit.

Thanks, amueller!

@amueller
Copy link
Member

Don't mutate the default argument in __init__. That won't work. You need to do it in fit. You don't need to run local tests, you should have to watch travis. Running tests locally is usually faster, though.

@bryandeng bryandeng force-pushed the immutable_defaults branch from a0afbe4 to bf41ad5 Compare March 19, 2015 00:00
@landscape-bot
Copy link

Code Health
Repository health decreased by 0.07% when pulling bf41ad5 on bryandeng:immutable_defaults into 81acb94 on scikit-learn:master.

@landscape-bot
Copy link

Code Health
Repository health decreased by 0.07% when pulling bf41ad5 on bryandeng:immutable_defaults into 81acb94 on scikit-learn:master.

@bryandeng
Copy link
Contributor Author

@amueller Can you restart the build later on?
I got strange error information about cross validation. But I didn't touch that part of code.
And #4368 shows that @xuewei4d was working on rfe.py so I reverted it to avoid merge conflict.

I built my fork repo on Travis and it passed.

@amueller
Copy link
Member

master was broken, sorry, my fault :-/

@amueller
Copy link
Member

I just restarted, should be fine now.

@amueller
Copy link
Member

Also, don't worry much about merge conflicts. We have 200 open PRs. It will be impossible not to conflict with any ;)

@bryandeng
Copy link
Contributor Author

Do I need to add some tests/documentation? Maybe I can help on #4379.

@amueller
Copy link
Member

#4379 will be mergable once everything is fixed.

@amueller amueller changed the title [MRG] Use immutable default arguments throughtout repo [MRG + 1] Use immutable default arguments throughtout repo Mar 19, 2015
@amueller
Copy link
Member

I don't think additional tests are needed. It looks good to me.

@bryandeng
Copy link
Contributor Author

OK. I'll now squash the commits into one for easier cherry-picking and start to work on other issues.

Replace mutable default arguments in examples

Replace mutable default arguments in sklearn/manifold/t_sne.py

Replace mutable default arguments in sklearn/metrics/pairwise.py
@bryandeng bryandeng force-pushed the immutable_defaults branch from bf41ad5 to 2638fe5 Compare March 19, 2015 15:34
@amueller
Copy link
Member

thanks :)

@landscape-bot
Copy link

Code Health
Repository health decreased by 0.07% when pulling 2638fe5 on bryandeng:immutable_defaults into bc5acea on scikit-learn:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0%) to 95.09% when pulling 2638fe5 on bryandeng:immutable_defaults into bc5acea on scikit-learn:master.

@ogrisel
Copy link
Member

ogrisel commented Mar 20, 2015

LGTM. Thanks @bryandeng!

ogrisel added a commit that referenced this pull request Mar 20, 2015
[MRG + 1] Use immutable default arguments throughtout repo
@ogrisel ogrisel merged commit aa60779 into scikit-learn:master Mar 20, 2015
@bryandeng bryandeng deleted the immutable_defaults branch March 20, 2015 12:42
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