Skip to content

[MRG + 1] TST test that all default arguments are not mutable #4379

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
May 18, 2015

Conversation

amueller
Copy link
Member

Check that default parameter values are of a basic type.
See #4376 for a fix for one of them, #4373 for the issue.

@amueller
Copy link
Member Author

I'm not sure about functions. They are mutable, but I guess they are fine, right?

@amueller
Copy link
Member Author

Should wait for #4713, then it will work.
I'm not sure if it is intentional that the memory objects are mutable defaults. @ogrisel @agramfort @GaelVaroquaux ?

@agramfort
Copy link
Member

I'm not sure if it is intentional that the memory objects are mutable
defaults. @ogrisel https://github.com/ogrisel @agramfort
https://github.com/agramfort @GaelVaroquaux
https://github.com/GaelVaroquaux ?

it's not supposed to be changed in the functions so I think it's fine...

@amueller amueller force-pushed the immutable_defaults branch from 4bbb52f to e1ede2f Compare May 13, 2015 13:54
@amueller
Copy link
Member Author

ok, should pass now.

@arjoly
Copy link
Member

arjoly commented May 18, 2015

Looks good.

@amueller amueller changed the title [MRG] TST test that all default arguments are not mutable [MRG + 1] TST test that all default arguments are not mutable May 18, 2015
@amueller
Copy link
Member Author

Merging as minor then.

@amueller amueller force-pushed the immutable_defaults branch from e1ede2f to 90d5ef1 Compare May 18, 2015 15:41
@amueller amueller merged commit 90d5ef1 into scikit-learn:master May 18, 2015
@amueller amueller deleted the immutable_defaults branch May 19, 2017 20:40
@oulenz
Copy link
Contributor

oulenz commented Jul 17, 2019

I just ran into this, because I created a custom estimator that takes an object of a custom class as a default.

I wonder whether this should be tested for at all. I understand that the original motivation was to remove mutable default arguments from scikit-learn, but that has been achieved.

Note that the test allows functions and Memory objects, which are mutable, because their state is not expected to change. Likewise, the state of the custom objects I use will not change after initialisation, but the test obviously cannot know about that.

@rth
Copy link
Member

rth commented Jul 17, 2019

Indeed, while we can check it internally in scikit-learn, I agree that it could be something you might want to opt out in contrib projects.

See #13969 and #14381 for a future possible workaround

@amueller
Copy link
Member Author

@oulenz A possibly overkill but straightforward solution would be to introduce an estimator tag allow_mutable_default that disables this test.

@oulenz
Copy link
Contributor

oulenz commented Jul 17, 2019

I guess it depends how fine-grained you envision the system of estimator tags. Alternatively, perhaps an opt-out list of tests for individual estimators could be a more general solution.

@amueller
Copy link
Member Author

amueller commented Jul 17, 2019

Or just not including this one in the non-strict mode as @rth suggested.

I think depending on names of tests is bad because multiple tests test the same thing or rely on similar assumptions and we'll get in a real mess if we try that.

We should encode our assumptions about the estimator, not what specific tests to run.

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