MNT deprecate attributes in Partial Least Squares module #18768

marenwestermann · 2020-11-05T18:24:36Z

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Deprecation of the attributes x_mean_, y_mean_, x_std_, y_std_ in module _pls.py

Any other comments?

adrinjalali · 2020-11-05T20:08:34Z

we're already deprecating a few attributes in 0.24 in these classes, and this PR deprecates more (which I'm in favor of), but there are a bunch of other attributes which can be made private as well, like, all of these maybe:

    x_weights_ : ndarray of shape (n_features, n_components)
        The left singular vectors of the cross-covariance matrices of each
        iteration.

    y_weights_ : ndarray of shape (n_targets, n_components)
        The right singular vectors of the cross-covariance matrices of each
        iteration.

    x_loadings_ : ndarray of shape (n_features, n_components)
        The loadings of `X`.

    y_loadings_ : ndarray of shape (n_targets, n_components)
        The loadings of `Y`.

    x_scores_ : ndarray of shape (n_samples, n_components)
        The transformed training samples.

    y_scores_ : ndarray of shape (n_samples, n_components)
        The transformed training targets.

    x_rotations_ : ndarray of shape (n_features, n_components)
        The projection matrix used to transform `X`.

    y_rotations_ : ndarray of shape (n_features, n_components)
        The projection matrix used to transform `Y`.

should we deprecate them and include them in the release? Kinda looks cleaner if they're all deprecated in the same release, WDYT @ogrisel @NicolasHug

NicolasHug · 2020-11-05T20:14:06Z

apart from the scores (already deprecated), the rest of the attributes are relevant and shouldn't be deprecated

adrinjalali · 2020-11-05T20:24:08Z

what do you mean by relevant? I mean, what's the difference between x_mean_ and and x_rotations_ for instance in your opinion?

NicolasHug · 2020-11-05T20:26:54Z

they all have specific meanings: https://scikit-learn.org/dev/modules/cross_decomposition.html#cross-decomposition

NicolasHug

Thanks @marenwestermann , looks good.

we'll also need a quick test making sure these are deprecated. You can parametrize with all estimators and with all attributes.

NicolasHug

thanks @marenwestermann , LGTM if green!

sklearn/cross_decomposition/tests/test_pls.py

NicolasHug · 2020-11-06T12:08:22Z

@adrinjalali to expand on my (probably too short) reply above re the attributes: I think they're more relevant than the means and stds because they can be used for some specific purposes and they can also be used for inspection. The rotations_ matrices are used to project the input into the transformed space, so they're basically the equivalent of the components_ attributes for PCA. The weights are also related to projections, and the loadings are related to the regression (these estimators can do both transform and predict.)

adrinjalali · 2020-11-06T12:14:42Z

@adrinjalali to expand on my (probably too short) reply above re the attributes: I think they're more relevant than the means and stds because they can be used for some specific purposes and they can also be used for inspection. The rotations_ matrices are used to project the input into the transformed space, so they're basically the equivalent of the components_ attributes for PCA. The weights are also related to projections, and the loadings are related to the regression (these estimators can do both transform and predict.)

fair point, that's convincing :)

marenwestermann · 2020-11-06T12:41:10Z

@NicolasHug I added the TODO note and removed the classes from the IGNORED set in sklearn/tests/test_docstring_parameters.py as suggested by @glemaitre in #18764 (I hope it's correct). Do I also need to leave a note in doc/whats_new/v0.24.rst saying that the attributes here are deprecated?

glemaitre · 2020-11-10T13:48:15Z

sklearn/cross_decomposition/tests/test_pls.py

@@ -488,6 +488,18 @@ def test_norm_y_weights_deprecation(Est):
        est.norm_y_weights


+# TODO: Remove test in 0.26
+@pytest.mark.parametrize('Est', (PLSRegression, PLSCanonical, CCA, PLSSVD))


Suggested change

@pytest.mark.parametrize('Est', (PLSRegression, PLSCanonical, CCA, PLSSVD))

@pytest.mark.parametrize('Estimator', (PLSRegression, PLSCanonical, CCA, PLSSVD))

glemaitre

We will need an entry in the what's new to announce the deprecation of the parameters since they could be used.

Otherwise, LGTM

glemaitre · 2020-11-10T13:51:01Z

sklearn/cross_decomposition/tests/test_pls.py

@@ -488,6 +488,18 @@ def test_norm_y_weights_deprecation(Est):
        est.norm_y_weights


+# TODO: Remove test in 0.26
+@pytest.mark.parametrize('Est', (PLSRegression, PLSCanonical, CCA, PLSSVD))
+@pytest.mark.parametrize('attr', ("x_mean_", "y_mean_", "x_std_", "y_std_"))


Suggested change

@pytest.mark.parametrize('attr', ("x_mean_", "y_mean_", "x_std_", "y_std_"))

@pytest.mark.parametrize('attribute', ("x_mean_", "y_mean_", "x_std_", "y_std_"))

glemaitre · 2020-11-10T13:51:01Z

sklearn/cross_decomposition/tests/test_pls.py

+    Y = rng.randn(10, 3)
+    est = Est().fit(X, Y)
+    with pytest.warns(FutureWarning, match=f"{attr} was deprecated"):
+        getattr(est, attr)


Suggested change

getattr(est, attr)

getattr(estimato, attribute)

glemaitre · 2020-11-10T13:51:01Z

sklearn/cross_decomposition/tests/test_pls.py

+    X = rng.randn(10, 5)
+    Y = rng.randn(10, 3)
+    est = Est().fit(X, Y)
+    with pytest.warns(FutureWarning, match=f"{attr} was deprecated"):


Suggested change

with pytest.warns(FutureWarning, match=f"{attr} was deprecated"):

with pytest.warns(FutureWarning, match=f"{attribute} was deprecated"):

glemaitre · 2020-11-10T13:51:01Z

sklearn/cross_decomposition/tests/test_pls.py

+    rng = np.random.RandomState(0)
+    X = rng.randn(10, 5)
+    Y = rng.randn(10, 3)
+    est = Est().fit(X, Y)


Suggested change

est = Est().fit(X, Y)

estimator = Estimator().fit(X, Y)

marenwestermann · 2020-11-11T10:58:38Z

I implemented the suggested changes and made an entry in the what's new section.

adrinjalali

Thanks @marenwestermann

adrinjalali · 2020-11-11T11:37:13Z

ping @ogrisel for the release

deprecate attributes

590137d

github-actions bot added the module:cross_decomposition label Nov 5, 2020

NicolasHug reviewed Nov 5, 2020

View changes

add test

9be81f4

NicolasHug approved these changes Nov 6, 2020

View changes

sklearn/cross_decomposition/tests/test_pls.py Outdated Show resolved Hide resolved

marenwestermann added 2 commits November 6, 2020 11:50

add remove test note

e1a75d6

remove classes from ignored sets

7be9c60

glemaitre reviewed Nov 10, 2020

View changes

marenwestermann added 2 commits November 10, 2020 16:31

replace abbreviations with full words for improved readability

c323aff

make entry in whats_new

271be9b

adrinjalali approved these changes Nov 11, 2020

View changes

adrinjalali merged commit 0e6d415 into scikit-learn:master Nov 11, 2020
7 checks passed

adrinjalali added this to the 0.24 milestone Nov 11, 2020

marenwestermann deleted the pls-attribute-deprecation branch November 11, 2020 11:59

MNT deprecate attributes in Partial Least Squares module #18768

MNT deprecate attributes in Partial Least Squares module #18768

marenwestermann commented Nov 5, 2020

adrinjalali commented Nov 5, 2020

NicolasHug commented Nov 5, 2020 •

edited

adrinjalali commented Nov 5, 2020

NicolasHug commented Nov 5, 2020

NicolasHug left a comment

NicolasHug left a comment

NicolasHug commented Nov 6, 2020 •

edited

adrinjalali commented Nov 6, 2020

marenwestermann commented Nov 6, 2020

glemaitre Nov 10, 2020

glemaitre left a comment

glemaitre Nov 10, 2020

glemaitre Nov 10, 2020

glemaitre Nov 10, 2020

glemaitre Nov 10, 2020

marenwestermann commented Nov 11, 2020

adrinjalali left a comment

adrinjalali commented Nov 11, 2020

	@pytest.mark.parametrize('Est', (PLSRegression, PLSCanonical, CCA, PLSSVD))
	@pytest.mark.parametrize('Estimator', (PLSRegression, PLSCanonical, CCA, PLSSVD))

	@pytest.mark.parametrize('attr', ("x_mean_", "y_mean_", "x_std_", "y_std_"))
	@pytest.mark.parametrize('attribute', ("x_mean_", "y_mean_", "x_std_", "y_std_"))

	with pytest.warns(FutureWarning, match=f"{attr} was deprecated"):
	with pytest.warns(FutureWarning, match=f"{attribute} was deprecated"):

MNT deprecate attributes in Partial Least Squares module #18768

MNT deprecate attributes in Partial Least Squares module #18768

Conversation

marenwestermann commented Nov 5, 2020

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Any other comments?

adrinjalali commented Nov 5, 2020

NicolasHug commented Nov 5, 2020 • edited

adrinjalali commented Nov 5, 2020

NicolasHug commented Nov 5, 2020

NicolasHug left a comment

Choose a reason for hiding this comment

NicolasHug left a comment

Choose a reason for hiding this comment

NicolasHug commented Nov 6, 2020 • edited

adrinjalali commented Nov 6, 2020

marenwestermann commented Nov 6, 2020

glemaitre Nov 10, 2020

Choose a reason for hiding this comment

glemaitre left a comment

Choose a reason for hiding this comment

glemaitre Nov 10, 2020

Choose a reason for hiding this comment

glemaitre Nov 10, 2020

Choose a reason for hiding this comment

glemaitre Nov 10, 2020

Choose a reason for hiding this comment

glemaitre Nov 10, 2020

Choose a reason for hiding this comment

marenwestermann commented Nov 11, 2020

adrinjalali left a comment

Choose a reason for hiding this comment

adrinjalali commented Nov 11, 2020

NicolasHug commented Nov 5, 2020 •

edited

NicolasHug commented Nov 6, 2020 •

edited