[MRG+1] Add check_is_fitted to non standard functions by whiletruelearn · Pull Request #12279 · scikit-learn/scikit-learn

whiletruelearn · 2018-10-04T14:45:10Z

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Adding check_is_fitted method to other non standard functions

Any other comments?

NicolasHug · 2018-10-04T14:58:26Z

Thanks @whiletruelearn , can you add a test to make sure that the correct exception is raised, like the code snippets in #12276?

sklearn/neighbors/tests/test_neighbors.py

NicolasHug · 2018-10-04T19:05:45Z

You can make sure the tests pass locally before pushing commits:

pytest sklearn/neighbors/tests/test_approximate.py

NicolasHug · 2018-10-04T19:07:03Z

test_graphs should not be failing as it calls lshf.fit, maybe this has to do with the use of ignore_warnings ?

whiletruelearn · 2018-10-04T19:10:19Z

@NicolasHug thanks . Had some trouble in setting up a proper dev environment. I think i have found the issue and fixed it with the last commit.

NicolasHug · 2018-10-04T21:22:59Z

sklearn/neighbors/base.py

        each object is a 1D array of indices or distances.
        """
-        check_is_fitted(self, "_fit_method")
+        check_is_fitted(self, "_fit_X")


Does

check_is_fitted(self, "_fit_method", "_fit_X")

still work? I think it would be best as it still ensure the previous test is covered

It works. Initially used _fit_method which was failing.

My understanding is that _fit_X is the attribute that needs to be checked for this functions.

I would leave check_is_fitted(self, "_fit_method", "_fit_X") then, it's safer and might cover some bugs that aren't caught in the tests

Okay @NicolasHug . Will update.

@NicolasHug I Tried adding this and tested locally. The build fails when i do so.

The current implementation looks correct to me .

Because in check_is_fitted

scikit-learn/sklearn/utils/validation.py

Line 944 in e6359e2

if not all_or_any([hasattr(estimator, attr) for attr in attributes]):

, exception is raised even if one of the attribute is not present. Please correct me if i am wrong.

You can use all_or_any=any

also made a slight change check_is_fitted(self, ["_fit_method", "_fit_X"], all_or_any=any)

whiletruelearn · 2018-10-05T08:17:34Z

@jnothman @NicolasHug Please review and let me know if i need to change something else.

whiletruelearn · 2018-10-08T05:37:29Z

Slight ping @jnothman @NicolasHug

jnothman · 2018-10-08T08:10:40Z

Please add a small |API| entry to the change log at doc/whats_new/v0.21.rst. Like the other entries there, please reference this pull request with :issue: and credit yourself (and other contributors if applicable) with :user:

whiletruelearn · 2018-10-08T09:52:22Z

Thanks. I have updated @jnothman

doc/whats_new/v0.21.rst

whiletruelearn · 2018-10-10T08:05:23Z

@jnothman @NicolasHug Kindly let me know if there are anymore changes to make. Thanks

NicolasHug · 2018-10-10T14:53:19Z

doc/whats_new/v0.21.rst

+  :func: `NearestNeighbors.kneighbors`,
+  :func: `NearestNeighbors.radius_neighbors`,
+  :func: `NearestNeighbors.kneighbors_graph`,
+  :func: `NearestNeighbors.radius_neighbors_graph`


I would suggest building the docs locally and checking the formatting is correct. Here you need to remove spaces after :func:

@NicolasHug Sorry , I have corrected this. I was hoping the build would capture issues if any. I did try make html, but it seems to be taking lots of time. What would be the right way to test a small change like this ?

NicolasHug · 2018-10-10T19:48:28Z

doc/whats_new/v0.21.rst

+  :func:`NearestNeighbors.kneighbors_graph`,
+  :func:`NearestNeighbors.radius_neighbors_graph`
+  without first fitting should raise `NotFittedError`.
+  :issue:`12279` by :user:`Krishna Sangeeth <whiletruelearn>`.


The formatting is still a bit messy. You can copy / paste this, it will do:

- |Fix| Fixed a bug in :class:`neighbors.NearestNeighbors` calling functions :func:`kneighbors <neighbors.NearestNeighbors.kneighbors>`, :func:`radius_neighbors <neighbors.NearestNeighbors.radius_neighbors>`, :func:`kneighbors_graph <neighbors.NearestNeighbors.kneighbors_graph>`, and :func:`radius_neighbors_graph <neighbors.NearestNeighbors.radius_neighbors_graph>` without first fitting should raise ``NotFittedError``. :issue:`12279` by :user:`Krishna Sangeeth <whiletruelearn>`.

Thanks @NicolasHug . I have updated.

NicolasHug · 2018-10-11T12:03:01Z

Thanks @whiletruelearn , LGTM!

jnothman · 2018-10-11T21:11:33Z

doc/whats_new/v0.21.rst

+......................
+
+- |Fix| Fixed a bug in :class:`neighbors.NearestNeighbors` calling functions
+  :func:`kneighbors <neighbors.NearestNeighbors.kneighbors>`,


:func:`kneighbors <neighbors.NearestNeighbors.kneighbors>`

is equivalent to

:func:`~neighbors.NearestNeighbors.kneighbors`

jnothman · 2018-10-11T21:13:02Z

doc/whats_new/v0.21.rst

+:mod:`sklearn.neighbors`
+......................
+
+- |Fix| Fixed a bug in :class:`neighbors.NearestNeighbors` calling functions


It's not a big bug. Let's phrase it as "Methods x x x now raise NotFittedError, rather than AttributeError, when called before fit". Can label it |API| too.

Thanks for correcting. I have updated @jnothman . Apologies for the delay.

whiletruelearn · 2018-10-15T05:48:15Z

Appveyor build seems to be failing for some reason. Should i create another issue for this.

[00:10:19] ================================== FAILURES ===================================
[00:10:19] _____________________________ test_count_nonzero ______________________________
[00:10:19] 
[00:10:19]     def test_count_nonzero():
[00:10:19]         X = np.array([[0, 3, 0],
[00:10:19]                       [2, -1, 0],
[00:10:19]                       [0, 0, 0],
[00:10:19]                       [9, 8, 7],
[00:10:19]                       [4, 0, 5]], dtype=np.float64)
[00:10:19]         X_csr = sp.csr_matrix(X)
[00:10:19]         X_csc = sp.csc_matrix(X)
[00:10:19]         X_nonzero = X != 0
[00:10:19]         sample_weight = [.5, .2, .3, .1, .1]
[00:10:19]         X_nonzero_weighted = X_nonzero * np.array(sample_weight)[:, None]
[00:10:19]     
[00:10:19]         for axis in [0, 1, -1, -2, None]:
[00:10:19]             assert_array_almost_equal(count_nonzero(X_csr, axis=axis),
[00:10:19]                                       X_nonzero.sum(axis=axis))
[00:10:19]             assert_array_almost_equal(count_nonzero(X_csr, axis=axis,
[00:10:19]                                                     sample_weight=sample_weight),
[00:10:19]                                       X_nonzero_weighted.sum(axis=axis))
[00:10:19]     
[00:10:19]         assert_raises(TypeError, count_nonzero, X_csc)
[00:10:19]         assert_raises(ValueError, count_nonzero, X_csr, axis=2)
[00:10:19]     
[00:10:19]         assert (count_nonzero(X_csr, axis=0).dtype ==
[00:10:19]                 count_nonzero(X_csr, axis=1).dtype)
[00:10:19]         assert (count_nonzero(X_csr, axis=0, sample_weight=sample_weight).dtype ==
[00:10:19]                 count_nonzero(X_csr, axis=1, sample_weight=sample_weight).dtype)
[00:10:19]     
[00:10:19]         # Check dtypes with large sparse matrices too
[00:10:19]         X_csr.indices = X_csr.indices.astype(np.int64)
[00:10:19]         X_csr.indptr = X_csr.indptr.astype(np.int64)
[00:10:19] >       assert (count_nonzero(X_csr, axis=0).dtype ==
[00:10:19]                 count_nonzero(X_csr, axis=1).dtype)
[00:10:19] 
[00:10:19] X          = array([[ 0.,  3.,  0.],
[00:10:19]        [ 2., -1.,  0.],
[00:10:19]        [ 0.,  0.,  0.],
[00:10:19]        [ 9.,  8.,  7.],
[00:10:19]        [ 4.,  0.,  5.]])
[00:10:19] X_csc      = <5x3 sparse matrix of type '<type 'numpy.float64'>'
[00:10:19] 	with 8 stored elements in Compressed Sparse Column format>
[00:10:19] X_csr      = <5x3 sparse matrix of type '<type 'numpy.float64'>'
[00:10:19] 	with 8 stored elements in Compressed Sparse Row format>
[00:10:19] X_nonzero  = array([[False,  True, False],
[00:10:19]        [ True,  True, False],
[00:10:19]        [False, False, False],
[00:10:19]        [ True,  True,  True],
[00:10:19]        [ True, False,  True]])
[00:10:19] X_nonzero_weighted = array([[0. , 0.5, 0. ],
[00:10:19]        [0.2, 0.2, 0. ],
[00:10:19]        [0. , 0. , 0. ],
[00:10:19]        [0.1, 0.1, 0.1],
[00:10:19]        [0.1, 0. , 0.1]])
[00:10:19] axis       = None
[00:10:19] sample_weight = [0.5, 0.2, 0.3, 0.1, 0.1]
[00:10:19]

jnothman · 2018-10-15T06:27:08Z

Sorry for the failing test. See #12382

whiletruelearn · 2018-10-16T03:03:59Z

@jnothman Slight ping. Anything else to look into :-)

jnothman

Just wait for another review, please

ogrisel

LGTM as well, merging. Thank you very much @whiletruelearn!

…2279)

…-learn#12279)" This reverts commit 48ceed0.

Add check_is_fitted to non standard functions

ac2aab0

Add test for checking whether the exception gets raised

b6e780d

whiletruelearn changed the title ~~Add check_is_fitted to non standard functions~~ [WIP] Add check_is_fitted to non standard functions Oct 4, 2018

NicolasHug reviewed Oct 4, 2018

View reviewed changes

sklearn/neighbors/tests/test_neighbors.py Outdated Show resolved Hide resolved

whiletruelearn added 3 commits October 4, 2018 21:41

swithch to simple assert raises exception

2c4f15f

Change the attribute name

acb0e88

change the signature of assert_raises method

69d8ff7

Missed to update for radius_neighbor

0c869d0

NicolasHug reviewed Oct 4, 2018

View reviewed changes

whiletruelearn changed the title ~~[WIP] Add check_is_fitted to non standard functions~~ [MRG] Add check_is_fitted to non standard functions Oct 5, 2018

whiletruelearn added 2 commits October 5, 2018 18:37

Add _fit_method also as attribute to check for

226e980

pass attributes as a list

6b7d96d

jnothman approved these changes Oct 8, 2018

View reviewed changes

Update what's new section

6207053

NicolasHug suggested changes Oct 8, 2018

View reviewed changes

doc/whats_new/v0.21.rst Outdated Show resolved Hide resolved

whiletruelearn added 2 commits October 8, 2018 18:13

Correct the indentation

7df6754

Update references to the functions

7075b2e

NicolasHug reviewed Oct 10, 2018

View reviewed changes

remove extra space

5d5cf31

NicolasHug reviewed Oct 10, 2018

View reviewed changes

fix formatting issues in doc

37bd344

NicolasHug approved these changes Oct 11, 2018

View reviewed changes

jnothman reviewed Oct 11, 2018

View reviewed changes

whiletruelearn added 2 commits October 14, 2018 22:43

Change in documentation as per review comments

3eebd21

Add a colon to separate

25362be

jnothman approved these changes Oct 16, 2018

View reviewed changes

whiletruelearn changed the title ~~[MRG] Add check_is_fitted to non standard functions~~ [MRG+1] Add check_is_fitted to non standard functions Oct 17, 2018

ogrisel approved these changes Oct 19, 2018

View reviewed changes

ogrisel merged commit 5bcd84b into scikit-learn:master Oct 19, 2018

finnoshea mentioned this pull request Feb 5, 2019

TST check_transformers_unfitted not run on some transformers #12967

Open

xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019

[MRG+1] Add check_is_fitted to non standard functions (scikit-learn#1…

48ceed0

…2279)

xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019

Revert "[MRG+1] Add check_is_fitted to non standard functions (scikit…

f2433c5

…-learn#12279)" This reverts commit 48ceed0.

xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019

Revert "[MRG+1] Add check_is_fitted to non standard functions (scikit…

3ca8795

…-learn#12279)" This reverts commit 48ceed0.

Uh oh!

Conversation

whiletruelearn commented Oct 4, 2018

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Any other comments?

Uh oh!

NicolasHug commented Oct 4, 2018

Uh oh!

Uh oh!

NicolasHug commented Oct 4, 2018

Uh oh!

NicolasHug commented Oct 4, 2018

Uh oh!

whiletruelearn commented Oct 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

whiletruelearn commented Oct 5, 2018

Uh oh!

whiletruelearn commented Oct 8, 2018

Uh oh!

jnothman commented Oct 8, 2018

Uh oh!

whiletruelearn commented Oct 8, 2018

Uh oh!

Uh oh!

whiletruelearn commented Oct 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NicolasHug commented Oct 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

whiletruelearn commented Oct 15, 2018

Uh oh!

jnothman commented Oct 15, 2018

Uh oh!

whiletruelearn commented Oct 16, 2018

Uh oh!

jnothman left a comment

Choose a reason for hiding this comment

Uh oh!

ogrisel left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants