8000 Fix numpy vstack on generator expressions by ogrisel · Pull Request #12467 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]

Skip to content

Fix numpy vstack on generator expressions #12467

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

Merged
merged 3 commits into from
Oct 29, 2018

Conversation

ogrisel
Copy link
Member
@ogrisel ogrisel commented Oct 26, 2018

This workaround should fix the test failing with numpy master (CRON job on travis):

https://travis-ci.org/scikit-learn/scikit-learn/builds/446632038

407     >>> from sklearn.cluster import SpectralBiclustering
408     >>> import numpy as np
409     >>> X = np.array([[1, 1], [2, 1], [1, 0],
410     ...               [4, 7], [3, 5], [3, 6]])
411     >>> clustering = SpectralBiclustering(n_clusters=2, random_state=0).fit(X)
UNEXPECTED EXCEPTION: ValueError('need at least one array to concatenate')
Traceback (most recent call last):
  File "/home/travis/miniconda/envs/testenv/lib/python3.7/doctest.py", line 1329, in __run
    compileflags, 1), test.globs)
  File "<doctest sklearn.cluster.bicluster.SpectralBiclustering[3]>", line 1, in <module>
  File "/home/travis/build/scikit-learn/scikit-learn/sklearn/cluster/bicluster.py", line 124, in fit
    self._fit(X)
  File "/home/travis/build/scikit-learn/scikit-learn/sklearn/cluster/bicluster.py", line 508, in _fit
    for label in range(n_row_clusters)
  File "/home/travis/miniconda/envs/testenv/lib/python3.7/site-packages/numpy/core/overrides.py", line 151, in public_api
    implementation, public_api, relevant_args, args, kwargs)
  File "/home/travis/miniconda/envs/testenv/lib/python3.7/site-packages/numpy/core/overrides.py", line 96, in array_function_implementation_or_override
    return implementation(*args, **kwargs)
  File "/home/travis/miniconda/envs/testenv/lib/python3.7/site-packages/numpy/core/shape_base.py", line 260, in vstack
    return _nx.concatenate([atleast_2d(_m) for _m in tup], 0)
  File "/home/travis/miniconda/envs/testenv/lib/python3.7/site-packages/numpy/core/overrides.py", line 151, in public_api
    implementation, public_api, relevant_args, args, kwargs)
  File "/home/travis/miniconda/envs/testenv/lib/python3.7/site-packages/numpy/core/overrides.py", line 96, in array_function_implementation_or_override
    return implementation(*args, **kwargs)
  File "/home/travis/miniconda/envs/testenv/lib/python3.7/site-packages/numpy/core/multiarray.py", line 193, in concatenate
    return _multiarray_umath.concatenate(arrays, axis, out)
ValueError: need at least one array to concatenate

@ogrisel
Copy link
Member Author
ogrisel commented Oct 26, 2018

oops, wrong branch. Let me rebase.

@amueller
Copy link
Member

shouldn't numpy avoid breaking changes? ;)

@ogrisel
Copy link
Member Author
ogrisel commented Oct 26, 2018

I agree but this is an easy workaround in the mean time.

@ogrisel ogrisel added this to the 0.20.1 milestone Oct 26, 2018
@ogrisel
Copy link
Member Author
ogrisel commented Oct 26, 2018

Note sure about the 0.20.1 milestone but probably safe to backport.

@amueller
Copy link
Member

ok but this change needs to be made in many more places, right?

@ogrisel
Copy link
Member Author
ogrisel commented Oct 26, 2018

Hum you are rights, I have just fixed a specific module but there are many others in the travis report. I reported the issue upstream. Let's see what they think of it.

I have to run now. Feel free to push the missing fix into that PR directly if you want.

@tylerjereddy
Copy link
Contributor

Related to numpy/numpy#12263 and the NEP implementing array function protocol.

For SciPy we did a similar fix by switching to list comprehensions.

@shoyer noted in the above issue the possibility of re-enabling the behavior for now, but eventually raising a TypeError

@amueller
Copy link
Member

I guess this is up for grabs if anyone wants to finish it as we should try to avoid the FutureWarnings.

@ogrisel
Copy link
Member Author
ogrisel commented Oct 29, 2018

I think I fixed them all (when running the tests locally with numpy master).

Miscellaneous
.............

- |Fix| Make sure to avoid raising ``FutureWarning`` when calling
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems that FutureWarning is only a proposal (not implemented) in numpy?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's pretty likely to be implemented. If not we can always update our changelog later.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you give me some pointers about where the proposal/discussion about FutureWarning and VisibleDeprecationWarning in numpy is happening?

It'll effect the semantics behind sklearn.utils.testing.assert_no_warnings: d6972a9

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see numpy/numpy#12263 mentioned above.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The FutureWarning PR has been merged into numpy master as well: https://github.com/numpy/numpy/pull/12280/files

@amueller
Copy link
Member

looks good. haven't checked for completeness but cron job will do that for us.

@amueller amueller merged commit e67e30c into scikit-learn:master Oct 29, 2018
@ogrisel ogrisel deleted the fix-vstack-genexp branch October 30, 2018 08:11
@qinhanmin2014
Copy link
Member

FYI, there's only a joblib issue in Travis cron job now :)

thoo pushed a commit to thoo/scikit-learn that referenced this pull request Nov 14, 2018
* Workaround vstack issue with genxp

* Use list comprehensions instead of genexps with np.vstack

* Add changelog entry.
jnothman pushed a commit to jnothman/scikit-learn that referenced this pull request Nov 14, 2018
* Workaround vstack issue with genxp

* Use list comprehensions instead of genexps with np.vstack

* Add changelog entry.
jnothman pushed a commit to jnothman/scikit-learn that referenced this pull request Nov 14, 2018
* Workaround vstack issue with genxp

* Use list comprehensions instead of genexps with np.vstack

* Add changelog entry.
xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019
* Workaround vstack issue with genxp

* Use list comprehensions instead of genexps with np.vstack

* Add changelog entry.
xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019
xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019
koenvandevelde pushed a commit to koenvandevelde/scikit-learn that referenced this pull request Jul 12, 2019
* Workaround vstack issue with genxp

* Use list comprehensions instead of genexps with np.vstack

* Add changelog entry.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants
0