[MRG] Use check_finite=False in sklearn.manifold #18886

jjerphan · Nov 20, 2020

Reference Issues/PRs

Treats #18837 for sklearn.manifold.

What does this implement/fix? Explain your changes.

It removes the check for non-finite values on scipy.linalg functions when previous checks exist in the sklearn.manifold module.

Any other comments?

For reference:

$ git grep linalg -- sklearn | grep import | grep scipy | grep -v cython_blas | grep manifold | grep -v test | grep -v sparse.linalg
sklearn/manifold/_locally_linear.py:from scipy.linalg import eigh, svd, qr, solve
sklearn/manifold/_spectral_embedding.py:from scipy.linalg import eigh
sklearn/manifold/_t_sne.py:from scipy import linalg

I gave some indications in each commit message. The only call that I haven't modified is this one:

scikit-learn/sklearn/manifold/_t_sne.py

Line 364 in 8d7d4c7

grad_norm = linalg.norm(grad)

This is possible as Gi is constructed from X which has been validated in `NeighborsBase._fit`.

This is possible as Ci is constructed from Gi which is constructed from X which has been validated in `NeighborsBase._fit`.

This is possible as Yi is finite by construction from the eigenvectors of Ci (which is symetric) contained in U.

X and Y contain finite values by assumption. Hence so do C, G and trace. v contain finite values by assumption. Thus we can remove the check on the call to `solve`.

This is possible as X_nbrs is constructed from X which has been validated in `NeighborsBase._fit`.

This is possible as C_nbrs is constructed from X which has been validated in `NeighborsBase._fit`.

This is possible as Xi is constructed from X which has been validated in `NeighborsBase._fit`.

This is possible as Ci is constructed from Gi which is constructed from X which has been validated in `NeighborsBase._fit`.

jjerphan · Nov 20, 2020

@mathschy: shall you be interested again, feel free to review.

sklearn/manifold/_locally_linear.py

mathschy · Nov 23, 2020

I just have 2 small uncertainties I do not know how to resolve myself, otherwise LGTM.

@jjerphan you may want to retry some checks in the CI ?

mathschy · Nov 23, 2020

@jjerphan actually I think you can also add a check_finite=False in line 333 of _spectral_embedding.py because the laplacian is already checked on line 325

Edit : I finished my review

jjerphan · Nov 23, 2020

@jjerphan you may want to retry some checks in the CI ?

It looks like the failure is unrelated to those changes — see the traces of #18883.

As @mathschy pointed out, `laplacian` is already verified line 325.

jjerphan · Nov 23, 2020

@jjerphan actually I think you can also add a check_finite=False in line 333 of _spectral_embedding.py because the laplacian is already checked on line 325

Thanks, I've updated it in 50621ed.

as reg might not be finite.

as eigh might return non-finite values.

jjerphan · Dec 15, 2020

I just have 2 small uncertainties I do not know how to resolve myself, otherwise LGTM.

@mathschy: I've reverted the checks as you proposed in e949029 and b2aeaba.

jjerphan · Dec 21, 2020

Hi @thomasjpfan, would you also be interested to review this pull request? 🙂

sklearn/manifold/_locally_linear.py

sklearn/manifold/_spectral_embedding.py

Those changes are made accordingly to @thomasjpfan's comments in scikit-learn#18886.

thomasjpfan

LGTM

jjerphan · Jan 21, 2021

@ogrisel: are you interested to review this (rather pretty short) PR? 🙂

…eck_manifold

rth

Thanks @jjerphan , LGTM.

jjerphan added 8 commits November 20, 2020 20:26

Use_check_finite=False for the call to svd (hessian)

bbb34f2

This is possible as Gi is constructed from X which has been validated in `NeighborsBase._fit`.

Use_check_finite=False for the call to eigh (hessian)

751376a

This is possible as Ci is constructed from Gi which is constructed from X which has been validated in `NeighborsBase._fit`.

Use_check_finite=False for the call to qr

b6e5ff1

This is possible as Yi is finite by construction from the eigenvectors of Ci (which is symetric) contained in U.

Use_check_finite=False for the call to solve

f76dae7

X and Y contain finite values by assumption. Hence so do C, G and trace. v contain finite values by assumption. Thus we can remove the check on the call to `solve`.

Use_check_finite=False for the call to svd (modified)

57f1afa

This is possible as X_nbrs is constructed from X which has been validated in `NeighborsBase._fit`.

Use_check_finite=False for the call to eigh (modified)

93c5d86

This is possible as C_nbrs is constructed from X which has been validated in `NeighborsBase._fit`.

Use_check_finite=False for the call to svd (ltsa)

be3f492

This is possible as Xi is constructed from X which has been validated in `NeighborsBase._fit`.

Use_check_finite=False for the call to eigh (ltsa)

ba7cd8b

This is possible as Ci is constructed from Gi which is constructed from X which has been validated in `NeighborsBase._fit`.

github-actions bot added the module:manifold label Nov 20, 2020

mathschy reviewed Nov 23, 2020

View reviewed changes

sklearn/manifold/_locally_linear.py Outdated Show resolved Hide resolved

mathschy reviewed Nov 23, 2020

View reviewed changes

sklearn/manifold/_locally_linear.py Outdated Show resolved Hide resolved

Use_check_finite=False for the call to eigh

50621ed

As @mathschy pointed out, `laplacian` is already verified line 325.

jjerphan added 2 commits December 12, 2020 10:59

Revert check_finite to True for solve

e949029

as reg might not be finite.

Revert check_finite to True for qr

b2aeaba

as eigh might return non-finite values.

jjerphan force-pushed the duplicate_finite_check_manifold branch from 6fcf933 to b2aeaba Compare December 12, 2020 10:00

thomasjpfan reviewed Dec 21, 2020

View reviewed changes

Revert check_finite to True for potential non-finiteness

b8fc12c

Those changes are made accordingly to @thomasjpfan's comments in scikit-learn#18886.

thomasjpfan approved these changes Dec 22, 2020

View reviewed changes

cmarmo added the Waiting for Reviewer label Jan 10, 2021

Base automatically changed from master to main January 22, 2021 10:53

Merge remote-tracking branch 'upstream/main' into duplicate_finite_ch…

9c60bcf

…eck_manifold

rth approved these changes Feb 3, 2021

View reviewed changes

rth merged commit 74a37de into scikit-learn:main Feb 3, 2021

jjerphan deleted the duplicate_finite_check_manifold branch February 4, 2021 07:31

glemaitre mentioned this pull request Apr 22, 2021

Release 0.24.2 #19954

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

[MRG] Use check_finite=False in sklearn.manifold #18886

[MRG] Use check_finite=False in sklearn.manifold #18886

Uh oh!

jjerphan commented Nov 20, 2020

Uh oh!

jjerphan commented Nov 20, 2020

Uh oh!

Uh oh!

Uh oh!

mathschy commented Nov 23, 2020

Uh oh!

mathschy commented Nov 23, 2020 •

edited

Loading

Uh oh!

jjerphan commented Nov 23, 2020

Uh oh!

jjerphan commented Nov 23, 2020

Uh oh!

jjerphan commented Dec 15, 2020

Uh oh!

jjerphan commented Dec 21, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thomasjpfan left a comment

Uh oh!

jjerphan commented Jan 21, 2021

Uh oh!

rth left a comment

Uh oh!

Uh oh!

Search code, repositories, users, issues, pull requests...

Uh oh!

[MRG] Use check_finite=False in sklearn.manifold #18886

[MRG] Use check_finite=False in sklearn.manifold #18886

Uh oh!

Conversation

jjerphan commented Nov 20, 2020

Uh oh!

jjerphan commented Nov 20, 2020

Uh oh!

Uh oh!

Uh oh!

mathschy commented Nov 23, 2020

Uh oh!

mathschy commented Nov 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jjerphan commented Nov 23, 2020

Uh oh!

jjerphan commented Nov 23, 2020

Uh oh!

jjerphan commented Dec 15, 2020

Uh oh!

jjerphan commented Dec 21, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thomasjpfan left a comment

Choose a reason for hiding this comment

Uh oh!

jjerphan commented Jan 21, 2021

Uh oh!

rth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mathschy commented Nov 23, 2020 •

edited

Loading