Skip to content

ENH Add dtype preservation to LocallyLinearEmbedding #24337

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.

Sign up for GitHub

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

Jump to bottom
Open
MisaOgura wants to merge 12 commits into
base: main
Choose a base branch
from
Open

ENH Add dtype preservation to LocallyLinearEmbedding #24337

MisaOgura wants to merge 12 commits into from

Conversation

@MisaOgura
Copy link
Contributor

@MisaOgura MisaOgura commented Sep 2, 2022

Reference Issues/PRs

In scope of #11000

What does this implement/fix? Explain your changes.

Implement dtype preservation to LocallyLinearEmbedding and relevant tests for various methods and solvers.

Any other comments?

@MisaOgura MisaOgura changed the title Add dtype preservation to LocallyLinearEmbedding ENH Add dtype preservation to LocallyLinearEmbedding Sep 2, 2022
@jeremiedbb
Copy link
Member

jeremiedbb commented Sep 2, 2022

Thanks for the PR @MisaOgura ! Looking quite good. Please add an entry in the v1.2.rst what's new, and then we'll wait for the CI to be green :)

@MisaOgura
Copy link
Contributor Author

MisaOgura commented Sep 3, 2022 •
edited
Loading

@jeremiedbb

I've looked into the failing tests on CI sklearn/manifold/tests/test_locally_linear.py::test_lle_manifold (apologies, I didn't realise that these tests were skipped when I ran the test suit locally) - the below are initial findings.

It seems that there is some instability with this test, as;

1. Different random seeds lead to different parameterised cases failing

e.g. running tests locally on M1

# default seed 0

sklearn/manifold/tests/test_locally_linear.py::test_lle_manifold[float32-dense-standard] FAILED *
sklearn/manifold/tests/test_locally_linear.py::test_lle_manifold[float32-dense-hessian] PASSED
sklearn/manifold/tests/test_locally_linear.py::test_lle_manifold[float32-dense-modified] FAILED *
sklearn/manifold/tests/test_locally_linear.py::test_lle_manifold[float32-dense-ltsa] PASSED

# with seed 105

sklearn/manifold/tests/test_locally_linear.py::test_lle_manifold[float32-dense-standard] PASSED
sklearn/manifold/tests/test_locally_linear.py::test_lle_manifold[float32-dense-hessian] PASSED
sklearn/manifold/tests/test_locally_linear.py::test_lle_manifold[float32-dense-modified] FAILED *
sklearn/manifold/tests/test_locally_linear.py::test_lle_manifold[float32-dense-ltsa] PASSED

2. Same random seeds lead to different parameterised cases failing on different platform

e.g. comparing tests on CI & locally, with the same default seed 0

# on CI

sklearn/manifold/tests/test_locally_linear.py::test_lle_manifold[float32-dense-standard] FAILED *
sklearn/manifold/tests/test_locally_linear.py::test_lle_manifold[float32-dense-hessian] FAILED *
sklearn/manifold/tests/test_locally_linear.py::test_lle_manifold[float32-dense-modified] PASSED
sklearn/manifold/tests/test_locally_linear.py::test_lle_manifold[float32-dense-ltsa] PASSED

# locally

sklearn/manifold/tests/test_locally_linear.py::test_lle_manifold[float32-dense-standard] FAILED *
sklearn/manifold/tests/test_locally_linear.py::test_lle_manifold[float32-dense-hessian] PASSED
sklearn/manifold/tests/test_locally_linear.py::test_lle_manifold[float32-dense-modified] FAILED *
sklearn/manifold/tests/test_locally_linear.py::test_lle_manifold[float32-dense-ltsa] PASSED

In both cases above, it is float32-dense-* cases that are affected by random seeds.

Cf.

A similar phenomenon is flagged in another test within the same file sklearn/manifold/tests/test_locally_linear.py::test_lle_simple_grid, but in this case it is explained that the flaky-ness due to ARPACK's numerically instability.

e.g. running tests locally on M1

# default seed 42

sklearn/manifold/tests/test_locally_linear.py::test_lle_simple_grid[float32-dense] PASSED
sklearn/manifold/tests/test_locally_linear.py::test_lle_simple_grid[float32-arpack] PASSED
sklearn/manifold/tests/test_locally_linear.py::test_lle_simple_grid[float64-dense] PASSED
sklearn/manifold/tests/test_locally_linear.py::test_lle_simple_grid[float64-arpack] PASSED

# with seed 0

sklearn/manifold/tests/test_locally_linear.py::test_lle_simple_grid[float32-dense] PASSED
sklearn/manifold/tests/test_locally_linear.py::test_lle_simple_grid[float32-arpack] FAILED *
sklearn/manifold/tests/test_locally_linear.py::test_lle_simple_grid[float64-dense] PASSED
sklearn/manifold/tests/test_locally_linear.py::test_lle_simple_grid[float64-arpack] PASSED

@MisaOgura MisaOgura force-pushed the 11000/preserve-dtype-locally-linear-embedding branch from 417a0e2 to cac4a3d Compare September 3, 2022 12:15
@glemaitre glemaitre self-requested a review September 5, 2022 15:50
@glemaitre
Copy link
Member

glemaitre commented Nov 3, 2022 •
edited
Loading

I just resolve the conflict. I will give a go to this PR.

glemaitre
glemaitre reviewed Nov 3, 2022
View reviewed changes
Copy link
Member

@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made a pass on the algorithm to check which structures are not yet preserved in np.float32.

Regarding the failure, I only think that we will have to define a more lenient tolerance for the 32-bit case.

I would like first to impose the bitness in the algorithm and then focus more specifically on the tests.

@MisaOgura Would you have time to carry on the changes?

sklearn/manifold/_locally_linear.py
"n_jobs": [None, Integral],
}

def _more_tags(self):
Copy link
Member

@glemaitre glemaitre Sep 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you move this method at the end of the class

sklearn/manifold/_locally_linear.py
return csr_matrix(
(data.ravel(), ind.ravel(), indptr),
shape=(n_samples, n_samples),
dtype=X.dtype,
Copy link
Member

@glemaitre glemaitre Nov 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no need to force the dtype here because data returned by barycenter_weights is preserving the dtype.

sklearn/manifold/_locally_linear.py
evals = np.zeros([N, nev], dtype=X.dtype)

# choose the most efficient way to find the eigenvectors
use_svd = n_neighbors > d_in
Copy link
Member

@glemaitre glemaitre Nov 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should modify the creation of tmp to avoid some later casting:

tmp = np.dot(V.transpose(0, 2, 1), np.ones(n_neighbors, dtype=X.dtype))

Copy link
Member

@glemaitre glemaitre Nov 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same with the initialization of w_reg:

w_reg = np.zeros((N, n_neighbors), dtype=X.dtype)

sklearn/manifold/_locally_linear.py
M = np.zeros((N, N), dtype=X.dtype)
for i in range(N):
s_i = s_range[i]

Copy link
Member

@glemaitre glemaitre Nov 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same for h in the loop below:

h = np.full(s_i, alpha_i, dtype=X.dtype) - np.dot(Vi.T, np.ones(n_neighbors, dtype=X.dtype))

sklearn/manifold/_locally_linear.py
M = np.zeros((N, N), dtype=X.dtype)

use_svd = n_neighbors > d_in

Copy link
Member

@glemaitre glemaitre Nov 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Below, we should initialize Gi:

Gi = np.zeros((n_neighbors, n_components + 1), dtype=X.dtype)

@jjerphan jjerphan mentioned this pull request Nov 15, 2022
Open
28 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@glemaitre glemaitre glemaitre left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

module:manifold

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@MisaOgura @jeremiedbb @glemaitre