Skip to content
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

[MRG] BUG Fixes test_scale_and_stability in windows #15661

Merged

Conversation

@thomasjpfan
Copy link
Member

thomasjpfan commented Nov 19, 2019

  1. Resolves error in test_scale_and_stability by using cond that was used in scipy <= 1.2 for pinv2.

  2. The install.cmd was adjusted to match the pip installed packages in building the wheels.

With this merged, we should be able to get MacPython/scikit-learn-wheels#35 to work and build wheels for 3.8

It is a little strange that the error did not always appear with scipy installed from conda.

thomasjpfan added 8 commits Nov 18, 2019
Copy link
Member

jnothman left a comment

This should be noted in what's new for users of recent scipy with scikit-learn 0.21

@rth

This comment has been minimized.

Copy link
Member

rth commented Nov 19, 2019

Thanks!

The install.cmd was adjusted to match the pip installed packages in building the wheels.

OK, but don't we still want to run tests with conda dependencies on Windows? What's the point of reproducing the CI for building wheels, if that is going to be tested when building wheels anyway?

@thomasjpfan

This comment has been minimized.

Copy link
Member Author

thomasjpfan commented Nov 19, 2019

I had to get the CI to use wheels to reproduce the error here. I’ll revert the back CI back to conda.

Copy link
Member

ogrisel left a comment

This looks good to me as well. @thomasjpfan have you tracked the PR in scipy that changed this behavior? What is the reason for this change?

@thomasjpfan

This comment has been minimized.

Copy link
Member Author

thomasjpfan commented Nov 19, 2019

This should be noted in what's new for users of recent scipy with scikit-learn 0.21

@jnothman Whats new in for 0.22.0 or 0.21.4?

@adrinjalali Can this be in the RC to get the windows wheel to build?

@thomasjpfan

This comment has been minimized.

Copy link
Member Author

thomasjpfan commented Nov 19, 2019

What is the reason for this change?

@ogrisel Here is the PR that made the change: scipy/scipy#10067

@adrinjalali

This comment has been minimized.

Copy link
Member

adrinjalali commented Nov 19, 2019

@adrinjalali Can this be in the RC to get the windows wheel to build?

I'll include it in RC3.

Interesting, then why is only windows which fails the test? It seems like here we're reverting the behavior to something which is not the desired solution, to pass the test. According the PR in scipy, the new bounds are a better choice. Therefore we should be using them, shouldn't we?

@thomasjpfan

This comment has been minimized.

Copy link
Member Author

thomasjpfan commented Nov 19, 2019

According the PR in scipy, the new bounds are a better choice. Therefore we should be using them, shouldn't we?

In principle this would be good to do in another PR. We would need to update _nipals_twoblocks_inner_loop to take into account the new behavior of pinv2.

Interesting, then why is only windows which fails the test?

This test has a history of failing on windows:

Related:

  • #14067 - We vendored the scipy<1.2 version of pinvh because of the condition. There was no way to set the condition in that situation without calculate the singular values before hand.
Copy link
Member

adrinjalali left a comment

Awesome, thanks @thomasjpfan . Feel free to merge once CI is green.

Also, since you know the details now, could you please also open an issue as you mentioned?

@thomasjpfan

This comment has been minimized.

Copy link
Member Author

thomasjpfan commented Nov 19, 2019

Added to whats_new to 0.22.

@jnothman

This comment has been minimized.

Copy link
Member

jnothman commented Nov 20, 2019

I suppose we could consider this for a 0.21.4... We want to release 0.21 for CPython 3.8 and maybe it's easiest if it also supports scipy 1.3.

@ogrisel ogrisel merged commit 0831dfb into scikit-learn:master Nov 20, 2019
21 checks passed
21 checks passed
LGTM analysis: C/C++ No code changes detected
Details
LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: Python No new or fixed alerts
Details
ci/circleci: deploy Your tests passed on CircleCI!
Details
ci/circleci: doc Your tests passed on CircleCI!
Details
ci/circleci: doc artifact Link to 0/doc/_changed.html
Details
ci/circleci: doc-min-dependencies Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 97.21%)
Details
codecov/project 97.22% (+<.01%) compared to 6024894
Details
scikit-learn.scikit-learn #20191119.31 succeeded
Details
scikit-learn.scikit-learn (Linting) Linting succeeded
Details
scikit-learn.scikit-learn (Linux py35_conda_openblas) Linux py35_conda_openblas succeeded
Details
scikit-learn.scikit-learn (Linux py35_ubuntu_atlas) Linux py35_ubuntu_atlas succeeded
Details
scikit-learn.scikit-learn (Linux pylatest_pip_openblas_pandas) Linux pylatest_pip_openblas_pandas succeeded
Details
scikit-learn.scikit-learn (Linux32 py35_ubuntu_atlas_32bit) Linux32 py35_ubuntu_atlas_32bit succeeded
Details
scikit-learn.scikit-learn (Linux_Runs pylatest_conda_mkl) Linux_Runs pylatest_conda_mkl succeeded
Details
scikit-learn.scikit-learn (Windows py35_pip_openblas_32bit) Windows py35_pip_openblas_32bit succeeded
Details
scikit-learn.scikit-learn (Windows py37_conda_mkl) Windows py37_conda_mkl succeeded
Details
scikit-learn.scikit-learn (macOS pylatest_conda_mkl) macOS pylatest_conda_mkl succeeded
Details
scikit-learn.scikit-learn (macOS pylatest_conda_mkl_no_openmp) macOS pylatest_conda_mkl_no_openmp succeeded
Details
@ogrisel

This comment has been minimized.

Copy link
Member

ogrisel commented Nov 20, 2019

Thanks @thomasjpfan, I merged with your targeted whats new at 0.22. @adrinjalali please add this to your to-backport list for 0.22.X and optionally 0.21.X if you feel like making a companion 0.21.4 release to add support for Python 3.8 as suggested by @jnothman.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.