Fix "Variables are collinear" warning in plots_lle_digits#14162
Fix "Variables are collinear" warning in plots_lle_digits#14162glemaitre merged 2 commits intoscikit-learn:masterfrom
Conversation
examples/manifold/plot_lle_digits.py
Outdated
| X2.flat[::X.shape[1] + 1] += 0.01 # Make X invertible | ||
| t0 = time() | ||
| X_lda = discriminant_analysis.LinearDiscriminantAnalysis(n_components=2).fit_transform(X2, y) | ||
| X_lda = discriminant_analysis.LinearDiscriminantAnalysis(n_components=2)\ |
There was a problem hiding this comment.
It's recommended to avoid \ for line breaks. A better way to fix this would be to do,
from sklearn.discriminant_analysis import LinearDiscriminantAnalysis
then
estimator = LineariscriminantAnalysis(n_components=2)
X_lda = estimator.fit_transform(X2, y)
if it's still too long
There was a problem hiding this comment.
Feedback noted. Let me work on it
examples/manifold/plot_lle_digits.py
Outdated
| n_components=2, random_state=0) | ||
| t0 = time() | ||
| # X3 = X.copy() | ||
| # X3.flat[::X.shape[1] + 1] += 0.01 # Make X invertible |
There was a problem hiding this comment.
Please, either add this code (if it is necessary and it solves some issues) or remove these added lines. Commented code is really confusing for the reader, particularly in examples.
There was a problem hiding this comment.
I left these lines inadvertently. I am removing them
| X2.flat[::X.shape[1] + 1] += 0.01 # Make X invertible | ||
| t0 = time() | ||
| X_lda = discriminant_analysis.LinearDiscriminantAnalysis(n_components=2).fit_transform(X2, y) | ||
| X_lda = discriminant_analysis.LinearDiscriminantAnalysis(n_components=2 |
There was a problem hiding this comment.
I see that there is a bit of inconsistency originally in the example. Could you do make sure that we have the following pattern for all the example:
estimator = module.Class(...)
t0 = time()
estimator.fit_transform(...)It will be nicer style for the line breaking then/
There was a problem hiding this comment.
Actually, let it as it is. We will open a new issue/PR to solve these inconsistencies.
glemaitre
left a comment
There was a problem hiding this comment.
LGTM. However, we need to open an issue to make the example consistent:
- importing only the class estimator on the top of the file (and not the module)
- declaring the estimator first and operate
fit_transformwith the timing around - correct that we apply TruncatedSVD and not PCA
|
Thanks @glemaitre Once we open the issue, I am willing to work on it. |
|
I am merging and opening an issue. Thanks @KenitoInc for your contribution. |
Fix this by assigning a value init='random' during initializing NeighborhoodComponentsAnalysis.
Related to issue #14117
#WiMLDS
@MaggieChege
@dominicondigo