Skip to content
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

[MRG+1] FIX Make sure GridSearchCV and RandomizedSearchCV are pickle-able #7594

Merged
merged 10 commits into from Oct 10, 2016

Conversation

Projects
None yet
3 participants
@raghavrv
Copy link
Member

raghavrv commented Oct 6, 2016

Fixes #7562

  • Subclasses the np.ma.MaskedArray and overrides the __getstate__ to make obj dtyped MaskedArrays pickle-able.
  • Uses this fixed utils.fixes.MaskedArray inside gs.cv_results_...

This is based off of numpy/numpy#8122

Please review @jnothman @amueller @GaelVaroquaux @davechallis

@raghavrv raghavrv added this to the 0.18.1 milestone Oct 6, 2016

@raghavrv raghavrv changed the title [MRG] FIX Make sure GridSearchCV and RandomizedSearchCV are picke-able [MRG] FIX Make sure GridSearchCV and RandomizedSearchCV are pickle-able Oct 6, 2016

@raghavrv

This comment has been minimized.

Copy link
Member Author

raghavrv commented Oct 6, 2016

[The flake8 test will fail because of the utils.fixes import]

@lesteve

This comment has been minimized.

Copy link
Member

lesteve commented Oct 7, 2016

[The flake8 test will fail because of the utils.fixes import]

If you really care you can do something like this to silence the warning:

from numpy.ma import MaskedArray  # noqa

Or we can just merge like this obviously.

@lesteve

lesteve approved these changes Oct 7, 2016

Copy link
Member

lesteve left a comment

Besides the minor comment, LGTM


if np_version < (1, 12, 0):
class MaskedArray(np.ma.MaskedArray):
# Before numpy 1.12, np.ma.MaskedArray object is not pickle-able

This comment has been minimized.

Copy link
@lesteve

lesteve Oct 7, 2016

Member

I would replace pickle-able by picklable which is an acceptable adjective I think (it is definitely already mentioned a few times in the source code).

This comment has been minimized.

Copy link
@raghavrv

raghavrv Oct 7, 2016

Author Member

Done!

@raghavrv

This comment has been minimized.

Copy link
Member Author

raghavrv commented Oct 7, 2016

from numpy.ma import MaskedArray # noqa

Ah... Cool. I recall seeing this somewhere!

@raghavrv

This comment has been minimized.

Copy link
Member Author

raghavrv commented Oct 7, 2016

Besides the minor comment, LGTM

Thanks for the review @lesteve!!

@amueller
Copy link
Member

amueller left a comment

Can you check whether the pickle tests in estimator_checks.py also load?


random_search = RandomizedSearchCV(clf, {'foo_param': [1, 2, 3]},
refit=True, n_iter=3)
random_search.fit(X, y)
pickle.dumps(random_search) # smoke test
pickle.loads(pickle.dumps(random_search))

This comment has been minimized.

Copy link
@amueller

amueller Oct 7, 2016

Member

maybe call predict afterwards?

@@ -940,12 +940,12 @@ def test_pickle():
clf = MockClassifier()
grid_search = GridSearchCV(clf, {'foo_param': [1, 2, 3]}, refit=True)
grid_search.fit(X, y)
pickle.dumps(grid_search) # smoke test
pickle.loads(pickle.dumps(grid_search))

This comment has been minimized.

Copy link
@amueller

amueller Oct 7, 2016

Member

maybe call predict afterwards?

@raghavrv raghavrv removed the Needs Review label Oct 9, 2016

@raghavrv raghavrv force-pushed the raghavrv:pickleable_masked_array branch from 5596417 to 82e4d0a Oct 10, 2016

@raghavrv

This comment has been minimized.

Copy link
Member Author

raghavrv commented Oct 10, 2016

@amueller Thanks for the review. Have rebased and addressed your comment... The meta tests don't seem to test pickle :/ I'll confirm tomorrow...

@raghavrv raghavrv changed the title [MRG] FIX Make sure GridSearchCV and RandomizedSearchCV are pickle-able [MRG+1] FIX Make sure GridSearchCV and RandomizedSearchCV are pickle-able Oct 10, 2016

@amueller

This comment has been minimized.

Copy link
Member

amueller commented Oct 10, 2016

LGTM

@amueller amueller merged commit 868a58b into scikit-learn:master Oct 10, 2016

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@raghavrv raghavrv deleted the raghavrv:pickleable_masked_array branch Oct 10, 2016

amueller added a commit to amueller/scikit-learn that referenced this pull request Oct 14, 2016

[MRG+1] FIX Make sure GridSearchCV and RandomizedSearchCV are pickle-…
…able (scikit-learn#7594)

* FIX Subclass a new MaskedArray which allows pickling even when dype=object

* TST unpickling too

* FIX Use MaskedArray from utils.fixes rather than from numpy

* FIX imports

* Don't assign a variable

* FIX np --> numpy

* Use tostring instead of tobytes for old numpy

* COSMIT pickle-able --> picklable

* use #noqa comment to turn off flake8

* TST/ENH Check if the pickled est's predict matches with the original one's

# Conflicts:
#	sklearn/utils/tests/test_fixes.py
@amueller

This comment has been minimized.

Copy link
Member

amueller commented Nov 14, 2016

needs a whatsnew :-/

afiodorov added a commit to unravelin/scikit-learn that referenced this pull request Apr 25, 2017

[MRG+1] FIX Make sure GridSearchCV and RandomizedSearchCV are pickle-…
…able (scikit-learn#7594)

* FIX Subclass a new MaskedArray which allows pickling even when dype=object

* TST unpickling too

* FIX Use MaskedArray from utils.fixes rather than from numpy

* FIX imports

* Don't assign a variable

* FIX np --> numpy

* Use tostring instead of tobytes for old numpy

* COSMIT pickle-able --> picklable

* use #noqa comment to turn off flake8

* TST/ENH Check if the pickled est's predict matches with the original one's

Sundrique added a commit to Sundrique/scikit-learn that referenced this pull request Jun 14, 2017

[MRG+1] FIX Make sure GridSearchCV and RandomizedSearchCV are pickle-…
…able (scikit-learn#7594)

* FIX Subclass a new MaskedArray which allows pickling even when dype=object

* TST unpickling too

* FIX Use MaskedArray from utils.fixes rather than from numpy

* FIX imports

* Don't assign a variable

* FIX np --> numpy

* Use tostring instead of tobytes for old numpy

* COSMIT pickle-able --> picklable

* use #noqa comment to turn off flake8

* TST/ENH Check if the pickled est's predict matches with the original one's

paulha added a commit to paulha/scikit-learn that referenced this pull request Aug 19, 2017

[MRG+1] FIX Make sure GridSearchCV and RandomizedSearchCV are pickle-…
…able (scikit-learn#7594)

* FIX Subclass a new MaskedArray which allows pickling even when dype=object

* TST unpickling too

* FIX Use MaskedArray from utils.fixes rather than from numpy

* FIX imports

* Don't assign a variable

* FIX np --> numpy

* Use tostring instead of tobytes for old numpy

* COSMIT pickle-able --> picklable

* use #noqa comment to turn off flake8

* TST/ENH Check if the pickled est's predict matches with the original one's

MLopez-Ibanez pushed a commit to MLopez-Ibanez/scikit-learn that referenced this pull request Feb 9, 2019

[MRG+1] FIX Make sure GridSearchCV and RandomizedSearchCV are pickle-…
…able (scikit-learn#7594)

* FIX Subclass a new MaskedArray which allows pickling even when dype=object

* TST unpickling too

* FIX Use MaskedArray from utils.fixes rather than from numpy

* FIX imports

* Don't assign a variable

* FIX np --> numpy

* Use tostring instead of tobytes for old numpy

* COSMIT pickle-able --> picklable

* use #noqa comment to turn off flake8

* TST/ENH Check if the pickled est's predict matches with the original one's

# Conflicts:
#	sklearn/utils/tests/test_fixes.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.