-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
ENH support float32 in SpectralEmbedding for LOBPCG and PyAMG solvers #21534
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
Conversation
Remove check_array call for float64, since both lobpcg function and pyamg package support float32 for a few years now; see #21321
test_spectral_embedding_two_components for dtype in [np.float32, np.float64] and @pytest.mark.parametrize("eigen_solver", ("arpack", "lobpcg", "amg"))
mistake fixed
added an entry
PR 21534 entered
… @pytest.mark.parametrize("dtype", ("np.float32", "np.float64")) added to test_spectral_embedding_precomputed_affinity
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's still needed to check the input values but we can change the checks to accept float32 data:
laplacian = check_array(laplacian, dtype=[np.float64, np.float32], accept_sparse=True)
trying everywhere: @pytest.mark.parametrize("dtype", (float32, float64))
@pytest.mark.parametrize("dtype", ("float32", "float64"))
formatting laplacian = check_array( laplacian, dtype=[np.float64, np.float32], accept_sparse=True )
black fails here, but passes on my local system. I have not seen this before. There is no way to fix it for me, since black here does not show the changes suggested. |
`import pyamg` fails lint
@glemaitre could you please propose a specific commit? |
@pytest.mark.skipif( is_pyamg_not_available, reason="PyAMG is not installed and thus we cannot test." ) replaces pytest.importorskip("pyamg")
This would be the test that I have in mind: @pytest.mark.parametrize(
"eigen_solver",
[
"arpack",
"lobpcg",
pytest.param("amg", marks=skip_if_no_pyamg),
],
)
@pytest.mark.parametrize("dtype", [np.float32, np.float64])
def test_spectral_embedding_preserves_dtype(eigen_solver, dtype):
"""Check that `SpectralEmbedding is preserving the dtype of the fitted
attribute and transformed data.
Ideally, this test should be covered by the common test
`check_transformer_preserve_dtypes`. However, this test only run
with transformers implementing `transform` while `SpectralEmbedding`
implements only `fit_transform`.
"""
X = S.astype(dtype)
se = SpectralEmbedding(
n_components=2, affinity="rbf", eigen_solver=eigen_solver, random_state=0
)
X_trans = se.fit_transform(X)
assert X_trans.dtype == dtype
assert se.embedding_.dtype == dtype
assert se.affinity_matrix_.dtype == dtype |
@glemaitre I'll try adding your new tests just above and #21534 (comment) after getting all the tests pass again with the new changes |
typos
2 new tests proposed by @glemaitre
int64 for labels Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
@glemaitre both your new tests pass. Does one of them cover already your suggestion #21534 (comment) ? This is the only one remains open as far as I can see? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One final nitpick but otherwise LGTM! Thank you very much for your perseverance :)
@ogrisel thanks for reviewing promptly! "Perseverance" is my nick name. Life is too short for our coding efforts to be wasted :-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks @lobpcg all good for me. |
@glemaitre @ogrisel - thanks for your help! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I was writing this review. 😄
X = random_state.rand(laplacian.shape[0], n_components + 1) | ||
X[:, 0] = dd.ravel() | ||
X = X.astype(laplacian.dtype) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wish it were possible to specify dtype when using numpy PRNG!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought the same. But we need to discuss the SLEP about the random state
@jjerphan - Thanks for taking over! |
…scikit-learn#21534) Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
…scikit-learn#21534) Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Remove check_array call for float64, since both lobpcg function and pyamg package support float32 for a few years now; see
#21321
Reference Issues/PRs
Fixes #21321.
What does this implement/fix? Explain your changes.
Remove check_array call for float64, since both lobpcg function and pyamg package support float32 for a few years now