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] Add test for __dict__ #7553

Merged
merged 3 commits into from Nov 5, 2016

Conversation

Projects
None yet
4 participants
@kiote
Contributor

kiote commented Oct 3, 2016

Reference Issue

see #7297

What does this implement/fix? Explain your changes.

Check that estimator does not change the state of dict during fit phase.

Any other comments?

I add "WIP = work in progress" for this PR, since I'm not sure it does what it should do. So please give me some feedback first :)

After this, I'll add checking for transform stage as well.

@amueller

This comment has been minimized.

Member

amueller commented Oct 3, 2016

We certainly change the __dict__ during fit. The issue is not to change it during any other method.

@kiote

This comment has been minimized.

Contributor

kiote commented Oct 5, 2016

Got it. Now I'm checking that __dict__ is not changing during transform.
But that made me have a lot of questions.

  1. I can see that not all estimators have transform method. For now, I just check if it has, is it okay?
  2. I see a lot of deprecation warning like this: "DeprecationWarning: Function transform is deprecated; Support to use estimators as feature selectors will be removed in version 0.19. Use SelectFromModel instead." Which made me think I shouldn't use transform anymore... Right?
  3. Here I had to skip some estimators, because data doesn't fit, or who knows why. Seems like dirty hack, not sure how should I do that in correct way.
@amueller

This comment has been minimized.

Member

amueller commented Oct 5, 2016

You should check any of transform, predict, predict_proba and decision_function. I don't think there's a model that has all of these, but you should check any of them if they are available (starting with transform is fine). You can just ignore the deprecation warnings. Check out how that is done in check_estimators.py. Why were the ones failing that you are passing? These are surprising to me.

@jnothman

This comment has been minimized.

Member

jnothman commented Oct 5, 2016

(Before deprecation of the feature selection transformation, there certainly should have been models that have all of these, such as logistic regression)

@kiote

This comment has been minimized.

Contributor

kiote commented Oct 12, 2016

okay, what I've done by now:

  1. add check_dict method to estimator_checks
  2. call this method in test_common for (almost) all estimators.

Can you please tell if it's closer to the desired result or not? Thanks!

@kiote

This comment has been minimized.

Contributor

kiote commented Oct 13, 2016

I've added some special cases for RANSACRegressor and SpectralBiclustering in the new check. but still need to skip SpectralCoclustering because of

ValueError: Found array with 0 feature(s) (shape=(23, 0)) while a minimum of 1 is required.

error. Any suggestions?

def check_dict(name, Estimator):
rnd = np.random.RandomState(0)
if name in ['SpectralCoclustering']:
return 0

This comment has been minimized.

@jnothman

jnothman Oct 13, 2016

Member

just "return", no 0.

The issues with SpectralCoclustering and SpectralBiclustering may be resolved in #6141

@@ -397,6 +398,39 @@ def check_dtype_object(name, Estimator):
@ignore_warnings
def check_dict(name, Estimator):

This comment has been minimized.

@jnothman

jnothman Oct 13, 2016

Member

Maybe check_dict_unchanged

if name in ['RANSACRegressor']:
X = 3 * rnd.uniform(size=(20, 3))
else:
X = 2 * rnd.uniform(size=(20, 3))

This comment has been minimized.

@jnothman

jnothman Oct 13, 2016

Member

We can't use 3 * in all cases?

This comment has been minimized.

@kiote

kiote Oct 14, 2016

Contributor

If I use 3 * for all, I get ValueError: _BinaryGaussianProcessClassifierLaplace supports only binary classification. y contains classes [0 1 2]. Which is sounds fair enough.

With 2 * for all I get on the other side ValueError: No inliers found, possible cause is setting residual_threshold (None) too low. for RANSACRegressor :(

set_testing_parameters(estimator)
if hasattr(estimator, "n_components"):
estimator.n_components = 3

This comment has been minimized.

@jnothman

jnothman Oct 13, 2016

Member

why 3? other tests with this parameter setting use 1.

This comment has been minimized.

@kiote

kiote Oct 14, 2016

Contributor

well yes, but for some reason with 1 I catch ValueError: n_best cannot be larger than n_components, but 3 > 1 from SpectralBiclustering

This comment has been minimized.

@amueller

amueller Oct 14, 2016

Member

Hm maybe SpectralBiclustering needs n_best=1?

This comment has been minimized.

@kiote

kiote Oct 17, 2016

Contributor

thanks, that helped!

estimator = Estimator()
set_testing_parameters(estimator)
if hasattr(estimator, "n_components"):

This comment has been minimized.

@jnothman

jnothman Oct 13, 2016

Member

*Hmm... I wonder if these should be in set_testing_parameters)

This comment has been minimized.

@kiote

kiote Oct 14, 2016

Contributor

Can't say anything here, I've seen the same part on several places, for example here: https://github.com/kiote/scikit-learn/blob/feature-test-__dict__/sklearn/utils/estimator_checks.py#L443

So, it looks like it could be moved there, do you think it should?

This comment has been minimized.

@amueller

amueller Oct 14, 2016

Member

yeah feel free to move it there.

for method in ["predict", "transform", "decision_function",
"predict_proba"]:
if hasattr(estimator, method):
dict_before = estimator.__dict__

This comment has been minimized.

@jnothman

jnothman Oct 13, 2016

Member

You need to perform .copy() at the end of this. Otherwise, in almost all cases, testing equivalence doesn't test that __dict__ is unchanged, as you're actually comparing an object to itself.

This comment has been minimized.

@kiote

kiote Oct 14, 2016

Contributor

haha good point, after the real check I have to skip two more estimators: 'NMF' and 'ProjectedGradientNMF', since they actually change __dict__ on given steps.

if hasattr(estimator, method):
dict_before = estimator.__dict__
getattr(estimator, method)(X)
assert_equal(estimator.__dict__, dict_before)

This comment has been minimized.

@jnothman

jnothman Oct 13, 2016

Member

use assert_dict_equal

@jnothman

This comment has been minimized.

Member

jnothman commented Oct 13, 2016

Thanks for working on this, though I suspect you will find many more failures when you correctly perform .copy()

rnd = np.random.RandomState(0)
if name in ['SpectralCoclustering']:
return 0
if name in ['SpectralCoclustering', 'NMF', 'ProjectedGradientNMF']:

This comment has been minimized.

@amueller

amueller Oct 14, 2016

Member

How do they change the dict? Please leave them in so we can see the error and fix them.

This comment has been minimized.

@kiote

kiote Oct 17, 2016

Contributor

Both NMF and ProjectedGradientNMF changes n_iter value.

With SpectralCoclustering situation is different, I just can't make it work. It fails with error:

ValueError: Found array with 0 feature(s) (shape=(23, 0)) while a minimum of 1 is required.

@ignore_warnings
def test_predict_does_not_change_estimator_state():

This comment has been minimized.

@amueller

amueller Oct 14, 2016

Member

I think you should add this test to the generator that generates all tests in estimator_checks.py and not add a test here. Though for working on it this might be easier.

This comment has been minimized.

@kiote

kiote Oct 17, 2016

Contributor

got it, I'll remove this from here afterwards then!

This comment has been minimized.

@amueller

amueller Oct 19, 2016

Member

looks good to me once you move this.

set_random_state(estimator, 1)
if name in ['SpectralBiclustering']:

This comment has been minimized.

@amueller

amueller Oct 17, 2016

Member

SpectralBiclustering doesn't take y? I guess that's fixed in #6141.

This comment has been minimized.

@kiote

kiote Oct 18, 2016

Contributor

yep! those changes helped (I merged with maniteja123:issue6126 branch to check), so I suppose I could remove this after #6141 merge, right?

@RPGOne

RPGOne approved these changes Oct 17, 2016

@kiote

This comment has been minimized.

Contributor

kiote commented Oct 18, 2016

Not really sure, what should I do with failing tests for NMF and ProjectedGradientNMF estimators. Should I try to fix them in the scope of current issue as well?

@jnothman

This comment has been minimized.

Member

jnothman commented Oct 18, 2016

what should I do with failing tests for NMF and ProjectedGradientNMF estimators. Should I try to fix them in the scope of current issue as well?

It would be acceptable to skip them for now (by checking for their name) and then creating a new issue for this to be solved.

@jnothman

Getting there!

@@ -312,6 +314,14 @@ def set_testing_parameters(estimator):
if not isinstance(estimator, ProjectedGradientNMF):
estimator.set_params(solver='cd')
if hasattr(estimator, "n_components"):

This comment has been minimized.

@jnothman

jnothman Oct 18, 2016

Member

It seems setting these parameters here is a bad idea. It severely affects other tests.

This comment has been minimized.

@kiote

kiote Oct 19, 2016

Contributor

Okay, I'll move that back to the concrete test.

How did you check that, btw?

This comment has been minimized.

@jnothman

jnothman Oct 19, 2016

Member

Click "details" next to "continuous-integration/travis-ci/pr" under "Some changes were not successful"

This comment has been minimized.

@kiote

kiote Oct 19, 2016

Contributor

removed

This comment has been minimized.

@kiote

kiote Oct 19, 2016

Contributor

oh I see, thanks!

@kiote

This comment has been minimized.

Contributor

kiote commented Oct 19, 2016

I added some comments right in the code to explain some things, also I'm gong to remove new code from tests/test_common.py when you approve this changes.

@jnothman

This comment has been minimized.

Member

jnothman commented Oct 19, 2016

This looks okay... could you change the title from WIP to MRG and put the code into mergeable form?

def check_dict_unchanged(name, Estimator):
# these two estimators change the state of __dict__
# and need to be fixed
if name in ['NMF', 'ProjectedGradientNMF']:

This comment has been minimized.

@amueller

amueller Oct 19, 2016

Member

Actually, it might be simpler to remove this line instead, that should get rid of the issue.

@kiote kiote changed the title from WIP: Add test for __dict__ to [MRG] Add test for __dict__ Oct 20, 2016

@kiote

This comment has been minimized.

Contributor

kiote commented Oct 20, 2016

done!

@amueller

This comment has been minimized.

Member

amueller commented Oct 20, 2016

can you please rebase? Otherwise LGTM.

@amueller amueller changed the title from [MRG] Add test for __dict__ to [MRG + 1] Add test for __dict__ Oct 20, 2016

@kiote

This comment has been minimized.

Contributor

kiote commented Oct 21, 2016

okay! now it's rebased from master

@amueller

This comment has been minimized.

Member

amueller commented Oct 21, 2016

Can you check the changed files as shown by github? I think something went wrong during the rebase.

@kiote

This comment has been minimized.

Contributor

kiote commented Oct 24, 2016

hmm, could you please tell more, what exactly do you mean? Can't see what's wrong there 😕

@amueller

This comment has been minimized.

Member

amueller commented Oct 24, 2016

@kiote never mind, I didn't have enough coffee...

@amueller

This comment has been minimized.

Member

amueller commented Oct 24, 2016

@jnothman wanna have another look?

@amueller

This comment has been minimized.

Member

amueller commented Oct 26, 2016

Can you please add a test to test_check_estimator that shows that check_estimator fails on an estimator that violates this?

@kiote

This comment has been minimized.

Contributor

kiote commented Oct 30, 2016

done, please see here

@jnothman

Otherwise LGTM

@@ -1106,7 +1106,6 @@ def transform(self, X):
nls_max_iter=self.nls_max_iter, sparseness=self.sparseness,
beta=self.beta, eta=self.eta)
self.n_iter_ = n_iter_

This comment has been minimized.

@jnothman

jnothman Oct 31, 2016

Member

Very awkwardly, there is an Attributes section in the docstring above (and similar ones in the docs for fit and fit_transform!). Please remove it. I suppose this change should also be documented ("Fixed bug where NMF's n_iter_ attribute was set by calls to transform"), though it's a very strange feature.

This comment has been minimized.

@kiote

kiote Nov 1, 2016

Contributor

I removed all self.n_iter = something in transform, but not really sure where this change should be documented. Could you please point me to the right place for that?

This comment has been minimized.

@jnothman

jnothman Nov 1, 2016

Member

Our changelog is in doc/whats_new.rst

getattr(estimator, method)(X)
assert_dict_equal(estimator.__dict__, dict_before,
('Estimator changes __dict__ during'
'predict, transform, decision_function'

This comment has been minimized.

@jnothman

jnothman Oct 31, 2016

Member

Could we just specify the one that's being tested?

This comment has been minimized.

@jnothman

jnothman Oct 31, 2016

Member

Or would we rather list all to make it clear what the API violation is?

This comment has been minimized.

@kiote

kiote Nov 1, 2016

Contributor

The idea was to list them all, even if only one violates the new rule, but it might be clearer to point directly to the one that's being tested.

This comment has been minimized.

@kiote

kiote Nov 1, 2016

Contributor

Changed here: 4bd07c7

@@ -988,6 +988,7 @@ def __init__(self, n_components=None, init=None, solver='cd',
self.l1_ratio = l1_ratio
self.verbose = verbose
self.shuffle = shuffle
self.n_iter_ = 1

This comment has been minimized.

@kiote

kiote Nov 1, 2016

Contributor

without having this attribute here I got

File "/home/travis/sklearn_build_latest/scikit-learn/sklearn/utils/estimator_checks.py", line 1600, in check_transformer_n_iter
assert_greater_equal(estimator.n_iter_, 1)
AttributeError: 'ProjectedGradientNMF' object has no attribute 'n_iter_'

from tests

This comment has been minimized.

@jnothman

jnothman Nov 2, 2016

Member

You can remove this now.

@jnothman

Sorry for the misunderstandings.

@@ -1021,9 +1021,6 @@ def fit_transform(self, X, y=None, W=None, H=None):
components_ : array-like, shape (n_components, n_features)
Factorization matrix, sometimes called 'dictionary'.
n_iter_ : int

This comment has been minimized.

@jnothman

jnothman Nov 1, 2016

Member

I was commenting that there should be no Attributes section at all in a method docstring. it belongs in the class docstring.

@@ -1049,7 +1046,6 @@ def fit_transform(self, X, y=None, W=None, H=None):
self.n_components_ = H.shape[0]
self.components_ = H
self.n_iter_ = n_iter_

This comment has been minimized.

@jnothman

jnothman Nov 1, 2016

Member

You should not have removed this. We need it in fit_transform, not in transform

This comment has been minimized.

@kiote

kiote Nov 2, 2016

Contributor

woops! placed back

This comment has been minimized.

@jnothman
@kiote

This comment has been minimized.

Contributor

kiote commented Nov 2, 2016

okay, now I put back some lines I removed accidentally with n_iter_, also removed docstrings you pointed to and add documentation part about this change (mostly about NMF change).

I wonder should I add doc about the new check as well?

@jnothman

Otherwise LGTM.

@@ -1066,9 +1059,6 @@ def fit(self, X, y=None, **params):
components_ : array-like, shape (n_components, n_features)

This comment has been minimized.

@jnothman

jnothman Nov 2, 2016

Member

Please drop all attributes here too

@@ -94,6 +94,11 @@ Bug fixes
(`#7680 <https://github.com/scikit-learn/scikit-learn/pull/7680>`_).
By `Ibraim Ganiev`_.
- Remove params changing inside of `transform` method of :class:`decomposition.NMF`

This comment has been minimized.

@jnothman

jnothman Nov 2, 2016

Member

The following would suffice:

    - Fixed a bug where :class:`decomposition.NMF` sets its `n_iters_`
      attribute in `transform()`. :issue:`7553` by `Ekaterina Krivich`_.

And under Enhancements, something like "check_estimator now attempts to ensure that methods transform, predict, etc. do not set attributes on the estimator."

@@ -988,6 +988,7 @@ def __init__(self, n_components=None, init=None, solver='cd',
self.l1_ratio = l1_ratio
self.verbose = verbose
self.shuffle = shuffle
self.n_iter_ = 1

This comment has been minimized.

@jnothman

jnothman Nov 2, 2016

Member

You can remove this now.

@@ -69,6 +69,11 @@ Enhancements
(`#7723 <https://github.com/scikit-learn/scikit-learn/pull/7723>`_)
by `Mikhail Korobov`_.
- ``check_estimator`` now attempts to ensure that methods transform, predict, etc.
do not set attributes on the estimator.
(`#7553 <https://github.com/scikit-learn/scikit-learn/pull/7553>`_)

This comment has been minimized.

@jnothman

jnothman Nov 3, 2016

Member

again, please use

:issue:`7533`

This comment has been minimized.

@kiote

kiote Nov 3, 2016

Contributor

okay, anyway nor my version, not this one is not clickable for me (looks like https://github.com/scikit-learn/scikit-learn/blob/master/doc/whats_new.rst#id17). Not the scope of this issue, obviously. I'll fix that!

(`#7553 <https://github.com/scikit-learn/scikit-learn/pull/7553>`_). Since it violates
new represented rule "estimator state does not change at transform/predict/predict_proba time".
By `Ekaterina Krivich`_.
- Fixed a bug where :class:`decomposition.NMF` sets its `n_iters_`

This comment has been minimized.

@jnothman

jnothman Nov 3, 2016

Member

to format n_iters_ correctly, you need double-backticks:

``n_iters_``
@jnothman

This comment has been minimized.

Member

jnothman commented Nov 3, 2016

Sigh. Can you update your master, rebase or merge, and fix conflicts in whats_new?

kiote added some commits Oct 3, 2016

Add test for __dict__ for estimator checks
  check that "predict", "transform", "decision_function" or
"predict_proba" methods do not change the state of __dict__ of any
estimator

  see #7297
Add a test to test_check_estimator
  that shows that check_estimator fails on an estimator that violates this
@kiote

This comment has been minimized.

Contributor

kiote commented Nov 4, 2016

oh, it's done

@jnothman

This comment has been minimized.

Member

jnothman commented Nov 5, 2016

Thanks!

@jnothman jnothman merged commit 02cc6f5 into scikit-learn:master Nov 5, 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

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

[MRG] TST Ensure __dict__ is unmodified by predict, transform, etc (s…
…cikit-learn#7553)

* Add test for __dict__ for estimator checks

  check that "predict", "transform", "decision_function" or
"predict_proba" methods do not change the state of __dict__ of any
estimator

  see scikit-learn#7297

* Add a test to test_check_estimator

  that shows that check_estimator fails on an estimator that violates this

* Fixed bug where NMF's n_iter_ attribute was set by calls to transform

sergeyf added a commit to sergeyf/scikit-learn that referenced this pull request Feb 28, 2017

[MRG] TST Ensure __dict__ is unmodified by predict, transform, etc (s…
…cikit-learn#7553)

* Add test for __dict__ for estimator checks

  check that "predict", "transform", "decision_function" or
"predict_proba" methods do not change the state of __dict__ of any
estimator

  see scikit-learn#7297

* Add a test to test_check_estimator

  that shows that check_estimator fails on an estimator that violates this

* Fixed bug where NMF's n_iter_ attribute was set by calls to transform

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

[MRG] TST Ensure __dict__ is unmodified by predict, transform, etc (s…
…cikit-learn#7553)

* Add test for __dict__ for estimator checks

  check that "predict", "transform", "decision_function" or
"predict_proba" methods do not change the state of __dict__ of any
estimator

  see scikit-learn#7297

* Add a test to test_check_estimator

  that shows that check_estimator fails on an estimator that violates this

* Fixed bug where NMF's n_iter_ attribute was set by calls to transform

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

[MRG] TST Ensure __dict__ is unmodified by predict, transform, etc (s…
…cikit-learn#7553)

* Add test for __dict__ for estimator checks

  check that "predict", "transform", "decision_function" or
"predict_proba" methods do not change the state of __dict__ of any
estimator

  see scikit-learn#7297

* Add a test to test_check_estimator

  that shows that check_estimator fails on an estimator that violates this

* Fixed bug where NMF's n_iter_ attribute was set by calls to transform

yarikoptic added a commit to yarikoptic/scikit-learn that referenced this pull request Jul 27, 2017

Merge tag '0.18.1' into releases
* tag '0.18.1': (144 commits)
  skip tree-test on 32bit
  do the warning test as we do it in other places.
  Replase assert_equal by assert_almost_equal in cosine test
  version bump 0.18.1
  fix merge conflict mess in whatsnew
  add the python2.6 warning to 0.18.1
  fix learning_curve test that I messed up in cherry-picking the "reentrant cv" PR.
  sync whatsnew with master
  [MRG] TST Ensure __dict__ is unmodified by predict, transform, etc (scikit-learn#7553)
  FIX scikit-learn#6420: Cloning decision tree estimators breaks criterion objects (scikit-learn#7680)
  Add whats new entry for scikit-learn#6282 (scikit-learn#7629)
  [MGR + 2] fix selectFdr bug (scikit-learn#7490)
  fixed whatsnew cherry-pick mess (somewhat)
  [MRG + 2] FIX LogisticRegressionCV to correctly handle string labels (scikit-learn#5874)
  [MRG + 2] Fixed parameter setting in SelectFromModel (scikit-learn#7764)
  [MRG+2] DOC adding separate `fit()` methods (and docstrings) for DecisionTreeClassifier and DecisionTreeRegressor (scikit-learn#7824)
  Fix docstring typo (scikit-learn#7844) n_features --> n_components
  [MRG + 1] DOC adding :user: role to whats_new (scikit-learn#7818)
  [MRG+1] label binarizer not used consistently in CalibratedClassifierCV (scikit-learn#7799)
  DOC : fix docstring of AIC/BIC in GMM
  ...

yarikoptic added a commit to yarikoptic/scikit-learn that referenced this pull request Jul 27, 2017

Merge branch 'releases' into dfsg
* releases: (144 commits)
  skip tree-test on 32bit
  do the warning test as we do it in other places.
  Replase assert_equal by assert_almost_equal in cosine test
  version bump 0.18.1
  fix merge conflict mess in whatsnew
  add the python2.6 warning to 0.18.1
  fix learning_curve test that I messed up in cherry-picking the "reentrant cv" PR.
  sync whatsnew with master
  [MRG] TST Ensure __dict__ is unmodified by predict, transform, etc (scikit-learn#7553)
  FIX scikit-learn#6420: Cloning decision tree estimators breaks criterion objects (scikit-learn#7680)
  Add whats new entry for scikit-learn#6282 (scikit-learn#7629)
  [MGR + 2] fix selectFdr bug (scikit-learn#7490)
  fixed whatsnew cherry-pick mess (somewhat)
  [MRG + 2] FIX LogisticRegressionCV to correctly handle string labels (scikit-learn#5874)
  [MRG + 2] Fixed parameter setting in SelectFromModel (scikit-learn#7764)
  [MRG+2] DOC adding separate `fit()` methods (and docstrings) for DecisionTreeClassifier and DecisionTreeRegressor (scikit-learn#7824)
  Fix docstring typo (scikit-learn#7844) n_features --> n_components
  [MRG + 1] DOC adding :user: role to whats_new (scikit-learn#7818)
  [MRG+1] label binarizer not used consistently in CalibratedClassifierCV (scikit-learn#7799)
  DOC : fix docstring of AIC/BIC in GMM
  ...

 Conflicts:  removed
	sklearn/externals/joblib/__init__.py
	sklearn/externals/joblib/_parallel_backends.py
	sklearn/externals/joblib/testing.py

yarikoptic added a commit to yarikoptic/scikit-learn that referenced this pull request Jul 27, 2017

Merge branch 'dfsg' into debian
* dfsg: (144 commits)
  skip tree-test on 32bit
  do the warning test as we do it in other places.
  Replase assert_equal by assert_almost_equal in cosine test
  version bump 0.18.1
  fix merge conflict mess in whatsnew
  add the python2.6 warning to 0.18.1
  fix learning_curve test that I messed up in cherry-picking the "reentrant cv" PR.
  sync whatsnew with master
  [MRG] TST Ensure __dict__ is unmodified by predict, transform, etc (scikit-learn#7553)
  FIX scikit-learn#6420: Cloning decision tree estimators breaks criterion objects (scikit-learn#7680)
  Add whats new entry for scikit-learn#6282 (scikit-learn#7629)
  [MGR + 2] fix selectFdr bug (scikit-learn#7490)
  fixed whatsnew cherry-pick mess (somewhat)
  [MRG + 2] FIX LogisticRegressionCV to correctly handle string labels (scikit-learn#5874)
  [MRG + 2] Fixed parameter setting in SelectFromModel (scikit-learn#7764)
  [MRG+2] DOC adding separate `fit()` methods (and docstrings) for DecisionTreeClassifier and DecisionTreeRegressor (scikit-learn#7824)
  Fix docstring typo (scikit-learn#7844) n_features --> n_components
  [MRG + 1] DOC adding :user: role to whats_new (scikit-learn#7818)
  [MRG+1] label binarizer not used consistently in CalibratedClassifierCV (scikit-learn#7799)
  DOC : fix docstring of AIC/BIC in GMM
  ...

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

[MRG] TST Ensure __dict__ is unmodified by predict, transform, etc (s…
…cikit-learn#7553)

* Add test for __dict__ for estimator checks

  check that "predict", "transform", "decision_function" or
"predict_proba" methods do not change the state of __dict__ of any
estimator

  see scikit-learn#7297

* Add a test to test_check_estimator

  that shows that check_estimator fails on an estimator that violates this

* Fixed bug where NMF's n_iter_ attribute was set by calls to transform

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

[MRG] TST Ensure __dict__ is unmodified by predict, transform, etc (s…
…cikit-learn#7553)

* Add test for __dict__ for estimator checks

  check that "predict", "transform", "decision_function" or
"predict_proba" methods do not change the state of __dict__ of any
estimator

  see scikit-learn#7297

* Add a test to test_check_estimator

  that shows that check_estimator fails on an estimator that violates this

* Fixed bug where NMF's n_iter_ attribute was set by calls to transform

maskani-moh added a commit to maskani-moh/scikit-learn that referenced this pull request Nov 15, 2017

[MRG] TST Ensure __dict__ is unmodified by predict, transform, etc (s…
…cikit-learn#7553)

* Add test for __dict__ for estimator checks

  check that "predict", "transform", "decision_function" or
"predict_proba" methods do not change the state of __dict__ of any
estimator

  see scikit-learn#7297

* Add a test to test_check_estimator

  that shows that check_estimator fails on an estimator that violates this

* Fixed bug where NMF's n_iter_ attribute was set by calls to transform
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment