FIX reshape the predicitons array shape in IterativeImputer by isaacknjama · Pull Request #19409 · scikit-learn/scikit-learn

isaacknjama · 2021-02-09T13:56:36Z

Reference Issues/PRs

Fixes #19352

What does this implement/fix? Explain your changes.

Fixes ValueError triggered when setting the estimator as PLSRegression().

jjerphan

What is the motive for using pytest context managers instead of sklearn.utils._testing.assert_raises?

glemaitre · 2021-02-09T15:38:50Z

What is the motive for using pytest context managers instead of sklearn.utils._testing.assert_raises?

It is more intuitive and readable since you have the real function call instead of wrapping the function and the parameter within another testing function.

The linked issue is wrong in the summary is wrong, I will correct it.

glemaitre · 2021-02-09T15:40:13Z

Uhm I see that you actually have changes from another PR (so my confusion regarding the original issue #14216 @jjerphan you can have a look).

glemaitre

As a general comment, when you start to make a PR, you should create a branch from the main branch such that no changes from other branches are already inside it.

sklearn/cluster/_kmeans.py

sklearn/tests/test_dummy.py

sklearn/impute/_iterative.py

glemaitre · 2021-02-09T15:47:00Z

Please add an entry to the change log at doc/whats_new/v*.rst. Like the other entries there, please reference this pull request with :pr: and credit yourself (and other contributors if applicable) with :user:.

jjerphan · 2021-02-09T15:47:04Z

It is more intuitive and readable since you have the real function call instead of wrapping the function and the parameter within another testing function.

I see, I thought using sklearn.utils._testing.assert_raises was a convention. Thanks!

jeremiedbb · 2021-02-09T15:49:01Z

it was before pytest was adopted :)
Now we want to slowly remove all these utilities

Mariam-ke · 2021-02-09T17:25:54Z

#DataUmbrella sprint
cc: @reshamas

isaacknjama · 2021-02-09T18:28:43Z

As a general comment, when you start to make a PR, you should create a branch from the main branch such that no changes from other branches are already inside it.

@glemaitre Thank you. I'll keep that in mind.

jjerphan · 2021-02-15T14:11:33Z

Hi @icky254

Why have you closed this PR? You were on a nice start. 🙂

sklearn/cluster/_kmeans.py

ogrisel · 2021-03-05T08:37:42Z

@icky254 are you still interested in finalizing this PR by addressing the remaining comments above? Let us know if something is not clear.

isaacknjama · 2021-03-05T08:48:05Z

@ogrisel I will finalize the PR. Thanks for the reminder.

Mariam-ke · 2021-03-11T14:18:51Z

@icky254 How is this PR going? Please let us know if we can answer any questions.

cc: @reshamas

This reverts commit 24c3497.

This reverts commit cfc8f28.

isaacknjama · 2021-03-16T15:41:50Z

The conflicting file in sklearn/tests/test_dummy.py causes builds to my PR to fail. I don't think it's required for this PR. How can I trace the commit that had the changes to the file?

jjerphan · 2021-04-09T14:25:47Z

doc/whats_new/v1.0.rst

+- |Fix| Fix ValueError triggered when setting the estimator as PLSRegression() in 
+  :class: `sklearn._iterative.py: func: _impute_one_feature`


Maybe a higher-level entry might be clearer to users?

Suggested change

- |Fix| Fix ValueError triggered when setting the estimator as PLSRegression() in

:class: `sklearn._iterative.py: func: _impute_one_feature`

- |Fix| :class:`cross_decomposition.PLSRegression` can now be used as an estimator for

:class:`impute.IterativeImputer`.

And we will need a non-regression test that should ensure that it is actually working in this PR while failing in the branch main

I got back to the original issue, and it looks like some issues (like SkuaD01/scikit-learn#1) implemented a test for this.

@isaack-mungui and @SkuaD01: you probably can work together to have your changes integrated in mean. 🙂

@jjerphan Just to gain clarity, should I integrate changes in mean or follow through with @glemaitre's requested changes first?

You can proceed with @glemaitre's request (I misspelled main and wrote mean).

Got it. Thanks!

glemaitre · 2021-04-09T14:50:21Z

doc/whats_new/v1.0.rst

+- |Fix| Fix ValueError triggered when setting the estimator as PLSRegression() in 
+  :class: `sklearn._iterative.py: func: _impute_one_feature`


And we will need a non-regression test that should ensure that it is actually working in this PR while failing in the branch main

reshamas · 2021-04-10T20:48:50Z

@isaack-mungui
If you would like to work with someone on this PR, you can ask on Data Umbrella Discord, and someone from our recent sprint will likely be available to collaborate.

Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>

isaacknjama · 2021-04-12T04:20:44Z

@isaack-mungui
If you would like to work with someone on this PR, you can ask on Data Umbrella Discord, and someone from our recent sprint will likely be available to collaborate.

@reshamas Noted, thanks.

sklearn/impute/tests/test_impute.py

Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>

jjerphan

LGTM, thanks @isaack-mungui!

isaacknjama · 2021-04-18T13:27:11Z

LGTM, thanks @isaack-mungui!

Thanks for the feedback. @jjerphan

cmarmo · 2021-05-26T16:23:08Z

@glemaitre do you mind having a look here to check if your requested changes have been applied? Thanks!
@isaack-mungui perhaps you can find sometime to synchronize with upstream? The continuous integration workflow has changed in the meanwhile and it is important to check that all builds are successful before merge. Thanks for your patience!

isaacknjama · 2021-05-26T18:10:19Z

@glemaitre do you mind having a look here to check if your requested changes have been applied? Thanks!
@isaack-mungui perhaps you can find sometime to synchronize with upstream? The continuous integration workflow has changed in the meanwhile and it is important to check that all builds are successful before merge. Thanks for your patience!

@cmarmo I'll do that ASAP. Thank you for the update!

isaacknjama added 5 commits February 6, 2021 13:03

replaced assert_raises with the pytest context manager

cfc8f28

replaced assert_raises with the pytest context manager

24c3497

Changed comment tolerance from independent to dependent

79c7ade

Updated comment on the tolerance function

ce405c9

Reshaped the interactive imputer by changing it to a 1D array

57b397e

github-actions bot added module:cluster module:impute labels Feb 9, 2021

jjerphan reviewed Feb 9, 2021

View reviewed changes

glemaitre reviewed Feb 9, 2021

View reviewed changes

sklearn/cluster/_kmeans.py Outdated Show resolved Hide resolved

sklearn/tests/test_dummy.py Outdated Show resolved Hide resolved

sklearn/impute/_iterative.py Show resolved Hide resolved

glemaitre changed the title ~~Imputer reshape~~ FIX reshape the predicitons array shape in IterativeImputer Feb 9, 2021

isaacknjama closed this Feb 13, 2021

isaacknjama deleted the imputer-reshape branch February 13, 2021 10:26

isaacknjama restored the imputer-reshape branch February 15, 2021 14:35

isaacknjama reopened this Feb 15, 2021

ogrisel reviewed Mar 5, 2021

View reviewed changes

sklearn/cluster/_kmeans.py Outdated Show resolved Hide resolved

Update sklearn/cluster/_kmeans.py

f7021cb

isaacknjama added 3 commits March 13, 2021 12:56

Added entry to file

7574207

Revert "replaced assert_raises with the pytest context manager"

6d93d29

This reverts commit 24c3497.

Called ravel on output of estimator.predict

369e67f

Revert "replaced assert_raises with the pytest context manager"

6a9acdb

This reverts commit cfc8f28.

Updated changes to username

6474b88

jjerphan reviewed Apr 9, 2021

View reviewed changes

glemaitre requested changes Apr 9, 2021

View reviewed changes

Update doc/whats_new/v1.0.rst

b14d80f

Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>

Added non-regression test

f7338fe

isaacknjama requested a review from glemaitre April 17, 2021 11:51

jjerphan reviewed Apr 18, 2021

View reviewed changes

sklearn/impute/tests/test_impute.py Outdated Show resolved Hide resolved

jjerphan reviewed Apr 18, 2021

View reviewed changes

sklearn/impute/tests/test_impute.py Outdated Show resolved Hide resolved

jjerphan reviewed Apr 18, 2021

View reviewed changes

sklearn/impute/tests/test_impute.py Outdated Show resolved Hide resolved

isaacknjama and others added 4 commits April 18, 2021 12:57

Updated redundant and missing imports

4e195a5

Update sklearn/impute/tests/test_impute.py

eb62c48

Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>

Update sklearn/impute/tests/test_impute.py

71b777c

Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>

Fixed unused variable error

31b6165

jjerphan approved these changes Apr 18, 2021

View reviewed changes

glemaitre mentioned this pull request Jul 22, 2021

FIX ensure to return 1-D array with single target in PLS* and CCA #20355

Closed

glemaitre removed their request for review July 22, 2021 08:13

cmarmo added the Superseded PR has been replace by a newer PR label Jul 25, 2021

glemaitre closed this Aug 4, 2021

		- \|Fix\| Fix ValueError triggered when setting the estimator as PLSRegression() in
		:class: `sklearn._iterative.py: func: _impute_one_feature`

Uh oh!

Conversation

isaacknjama commented Feb 9, 2021

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Uh oh!

jjerphan left a comment

Choose a reason for hiding this comment

Uh oh!

glemaitre commented Feb 9, 2021

Uh oh!

glemaitre commented Feb 9, 2021

Uh oh!

glemaitre left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

glemaitre commented Feb 9, 2021

Uh oh!

jjerphan commented Feb 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jeremiedbb commented Feb 9, 2021

Uh oh!

Mariam-ke commented Feb 9, 2021

Uh oh!

isaacknjama commented Feb 9, 2021

Uh oh!

jjerphan commented Feb 15, 2021

Uh oh!

Uh oh!

ogrisel commented Mar 5, 2021

Uh oh!

isaacknjama commented Mar 5, 2021

Uh oh!

Mariam-ke commented Mar 11, 2021

Uh oh!

isaacknjama commented Mar 16, 2021

Uh oh!

jjerphan Apr 9, 2021

Choose a reason for hiding this comment

Uh oh!

glemaitre Apr 9, 2021

Choose a reason for hiding this comment

Uh oh!

jjerphan Apr 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

isaacknjama Apr 13, 2021

Choose a reason for hiding this comment

Uh oh!

jjerphan Apr 13, 2021

Choose a reason for hiding this comment

Uh oh!

isaacknjama Apr 13, 2021

Choose a reason for hiding this comment

Uh oh!

glemaitre Apr 9, 2021

Choose a reason for hiding this comment

Uh oh!

reshamas commented Apr 10, 2021

Uh oh!

isaacknjama commented Apr 12, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jjerphan left a comment

Choose a reason for hiding this comment

Uh oh!

isaacknjama commented Apr 18, 2021

Uh oh!

cmarmo commented May 26, 2021

Uh oh!

isaacknjama commented May 26, 2021

Uh oh!

Reviewers

jjerphan commented Feb 9, 2021 •

edited

Loading

jjerphan Apr 9, 2021 •

edited

Loading