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+1] MAINT drop SciPy < 0.13 #8854

Merged
merged 8 commits into from Jun 2, 2017

Conversation

Projects
None yet
4 participants
@naoyak
Contributor

naoyak commented May 10, 2017

Ref: #8810

Removes or deprecates backport code covering scipy < 0.13.

  • utils.sparsetools.connected_components: deprecate and replace backport with wrapper (scipy 0.11+)
  • utils.fixes.expit: remove (scipy 0.10+)
  • utils.extmath.logsumexp: deprecate and replace backport (scipy 0.11+)
  • utils.fixes.rankdata: remove / utils.stats.rankdata: deprecate and replace backport (scipy 0.13+)
  • utils.arpack: deprecate and replace backport of sparse matrix functions (scipy 0.10+)
  • utils.extmath.norm: deprecate and replace backport (scipy 0.10+)
  • utils.exmath.pinvh: deprecate and replace backport (scipy 0.10+)
  • remove other version checks for scipy < 0.13 in examples and library code
@jnothman

This comment has been minimized.

Member

jnothman commented May 10, 2017

@jnothman

This comment has been minimized.

Member

jnothman commented May 10, 2017

@naoyak

This comment has been minimized.

Contributor

naoyak commented May 11, 2017

annoyingly i think we'll need to deprecate some of those things from utils.
Perhaps not from fixes, but if you're removing things from elsewhere...

Even plain backports from scipy (which the docs warn could disappear without notice)? I guess there is value in being strict about the deprecation contract...

@jnothman

This comment has been minimized.

Member

jnothman commented May 11, 2017

It hardly hurts to deprecate some of these things: delete any documentation that references them, but attach a deprecated decorator. The only annoyance is a potential drop in test coverage.

Where do we warn that something could disappear without notice?

@naoyak

This comment has been minimized.

Contributor

naoyak commented May 11, 2017

Where do we warn that something could disappear without notice?

My mistake, that's certainly not what the docs say:

Warning
These utilities are meant to be used internally within the scikit-learn package. They are not guaranteed to be stable between versions of scikit-learn. Backports, in particular, will be removed as the scikit-learn dependencies evolve.

I'll go back and add some deprecations. Something like:

@deprecated("sklearn.utils.extmath.logsumexp was deprecated in version 0.19 and will be "
            "removed in 0.21. Use scipy.misc.logsumexp instead.")
@jnothman

This comment has been minimized.

Member

jnothman commented May 11, 2017

@lesteve

This comment has been minimized.

Member

lesteve commented May 11, 2017

Update .travis.yml: SCIPY_VERSION="0.11" should become SCIPY_VERSION="0.13.1".

@lesteve

This comment has been minimized.

Member

lesteve commented May 11, 2017

Also please fix the flake8 errors in the Travis build. They looks like they are genuine problems.

@naoyak

This comment has been minimized.

Contributor

naoyak commented May 11, 2017

This is dependent on #8855.

Should we also replace calls to utils.extmath.norm with scipy.misc.norm at this point? @fabianp

@lesteve

This comment has been minimized.

Member

lesteve commented May 11, 2017

This is dependent on #8855.

I see I edited the issue to make this more clear.

@naoyak

This comment has been minimized.

Contributor

naoyak commented May 11, 2017

I wiped out as much old code as possible while preserving public API methods (except those in fixes). Once the CI dependencies on the other PR are worked out I'll rebase and see how it goes..

@naoyak

This comment has been minimized.

Contributor

naoyak commented May 13, 2017

Travis is now passing on the builds with scipy 0.17.0/0.18.1. Rebasing once #8855 is merged.

@naoyak naoyak changed the title from [WIP] MAINT drop SciPy < 0.13 to MAINT drop SciPy < 0.13 May 13, 2017

@naoyak naoyak referenced this pull request May 13, 2017

Merged

[MRG+1] Drop NumPy < 1.8 #8874

@naoyak naoyak changed the title from MAINT drop SciPy < 0.13 to [MRG] MAINT drop SciPy < 0.13 May 14, 2017

@naoyak

This comment has been minimized.

Contributor

naoyak commented May 14, 2017

This is ready for review. I stuck an extra commit on to test that CI passes with the proposed changes from #8855 but will later rebase.

The same commits are also in #8874 but I figure the diff is big enough without the NumPy-related changes to examine at once.

logX = np.vstack([logx, logx])
assert_array_almost_equal(np.exp(logsumexp(logX, axis=0)), X.sum(axis=0))
assert_array_almost_equal(np.exp(logsumexp(logX, axis=1)), X.sum(axis=1))

This comment has been minimized.

@naoyak

naoyak May 15, 2017

Contributor

question: when deprecating a backport (and replacing the implementation with a wrapper of the upstream function), should the test be left in place until we remove it entirely?

@jnothman

This comment has been minimized.

Member

jnothman commented May 15, 2017

@naoyak

This comment has been minimized.

Contributor

naoyak commented May 17, 2017

Added back the relevant tests, but now CircleCI erroring due to Yahoo Finance API being down

@naoyak naoyak closed this May 18, 2017

@naoyak naoyak reopened this May 18, 2017

@naoyak

This comment has been minimized.

Contributor

naoyak commented May 24, 2017

Now that #8855 is merged, this should be ready for a full review. The main point is whether the tests covering the deprecated functions are handled appropriately.

cc @jnothman

@lesteve

Quick first glance at this. It looks quite good overall.

@@ -1,23 +0,0 @@
from sklearn.utils.testing import assert_array_equal

This comment has been minimized.

@lesteve

lesteve Jun 1, 2017

Member

I think you should keep this test and add ignore_warnings like you did for other deprecated wrappers

This comment has been minimized.

@naoyak

naoyak Jun 2, 2017

Contributor

added back

from scipy.stats import rankdata as _sp_rankdata
from .fixes import bincount
from ..utils.extmath import stable_cumsum
from scipy.stats import rankdata as _rankdata

This comment has been minimized.

@lesteve

lesteve Jun 1, 2017

Member

Weird naming, I would do something like this:

from scipy.stats import rankdata as scipy_rankdata

@deprecated(...)
def rankdata(...):
    return scipy_rankdata(...)
###############################################################################
# Generate data
try:
face = sp.face(gray=True)
except AttributeError:
# Newer versions of scipy have face in misc
# SciPy 0.16 and higher have face in misc

This comment has been minimized.

@lesteve

lesteve Jun 1, 2017

Member

A bit of a nitpick but here and in other places where this code has been copied and pasted, I would do it the other way around:

try:
    # code that works with recent scipy versions
except AttributeError:
    # fall-back for old scipy versions

This comment has been minimized.

@lesteve

lesteve Jun 1, 2017

Member

Also I prefer using a math sign to make it more clear whether 0.16 is included or not, e.g. something like:

feature bla does not exist in Scipy < 0.13.2
from ._traversal import connected_components
from scipy.sparse.csgraph import connected_components as _connected_components # noqa

This comment has been minimized.

@lesteve

lesteve Jun 1, 2017

Member

Same remark about naming as before, I'd rather use scipy_connected_components.

This comment has been minimized.

@lesteve

lesteve Jun 1, 2017

Member

Why the #noqa by the way?

@@ -18,8 +18,9 @@
import numpy as np
from scipy import linalg
from scipy.sparse import issparse, csr_matrix
from scipy.misc import logsumexp as _logsumexp

This comment has been minimized.

@lesteve

lesteve Jun 1, 2017

Member

Same remark about naming here: use scipy_logsumexp

def norm(x):
@deprecated("sklearn.utils.extmath.norm was deprecated in version 0.19"
"and will be removed in 0.21. Use scipy.linalg.norm instead.")
def norm(x, *args, **kwargs):

This comment has been minimized.

@lesteve

lesteve Jun 1, 2017

Member

In all wrappers you deprecated I would keep the same signature as the original function. I don't really see the point of using *args and **kwargs ...

This comment has been minimized.

@naoyak

naoyak Jun 1, 2017

Contributor

The function signature for norm() changed over scipy versions, so I'm just passing the args to the original function. WDYT?

This comment has been minimized.

@naoyak

naoyak Jun 2, 2017

Contributor

For instance:

scipy 0.13
scipy.linalg.norm(a, ord=None)

scipy 0.19
scipy.linalg.norm(a, ord=None, axis=None, keepdims=False)

This comment has been minimized.

@jnothman

jnothman Jun 2, 2017

Member

Given that the former version only took x, I think you could keep it only taking `x``. But I'm okay with this.

This comment has been minimized.

@lesteve

lesteve Jun 2, 2017

Member

For the deprecated wrappers I would keep an identical signature as before.

This comment has been minimized.

@naoyak

naoyak Jun 2, 2017

Contributor

ok, fixed!

@naoyak

This comment has been minimized.

Contributor

naoyak commented Jun 1, 2017

Thanks @lesteve - feedback reflected!

@jakevdp

This comment has been minimized.

Member

jakevdp commented Jun 1, 2017

I think that many of the csgraph tools could be deprecated as well, e.g. most of what is in https://github.com/scikit-learn/scikit-learn/blob/4d9a12d175a38f2bcb720389ad2213f71a3d7697/sklearn/utils/sparsetools/_graph_tools.pyx is a direct backport from SciPy 0.11's version of the same routines: https://github.com/scipy/scipy/blob/master/scipy/sparse/csgraph/_tools.pyx

Edit: I think the validation code in https://github.com/scikit-learn/scikit-learn/blob/4d9a12d175a38f2bcb720389ad2213f71a3d7697/sklearn/utils/sparsetools/_graph_validation.py is similar.

Edit again: nevermind, it looks like you already got those 😄

@naoyak

This comment has been minimized.

Contributor

naoyak commented Jun 1, 2017

@jakevdp thanks for chiming in. Only connected_components is being used from sparsetools, so I'm replacing it with a wrapper for the scipy version, and deleting the related Cython files entirely. Does that seem right?

def norm(x):
@deprecated("sklearn.utils.extmath.norm was deprecated in version 0.19"
"and will be removed in 0.21. Use scipy.linalg.norm instead.")
def norm(x, *args, **kwargs):

This comment has been minimized.

@jnothman

jnothman Jun 2, 2017

Member

Given that the former version only took x, I think you could keep it only taking `x``. But I'm okay with this.

@@ -70,12 +70,6 @@ def single_source_shortest_path_length(graph, source, cutoff=None):
return seen # return all path lengths as dictionary
if hasattr(sparse, 'connected_components'):

This comment has been minimized.

@jnothman

jnothman Jun 2, 2017

Member

We maybe need to deprecate the presence of connected_components in this module.

This comment has been minimized.

@naoyak

naoyak Jun 2, 2017

Contributor

good catch

@@ -9,7 +9,6 @@ def configuration(parent_package='', top_path=None):
from numpy.distutils.misc_util import Configuration
config = Configuration('utils', parent_package, top_path)
config.add_subpackage('sparsetools')

This comment has been minimized.

@jnothman

jnothman Jun 2, 2017

Member

Now that we're deprecating, do we need to keep this?

This comment has been minimized.

@naoyak

naoyak Jun 2, 2017

Contributor

ah, we probably do.

@jnothman

This comment has been minimized.

Member

jnothman commented Jun 2, 2017

This LGTM... I hope I've not made any mistaken assumptions.

@jnothman jnothman changed the title from [MRG] MAINT drop SciPy < 0.13 to [MRG+1] MAINT drop SciPy < 0.13 Jun 2, 2017

@@ -1,22 +1,15 @@
import numpy
# Remove in version 0.21

This comment has been minimized.

@lesteve

lesteve Jun 2, 2017

Member

Given that there is no tests anymore, I think you can remove the tests folder and setup.py as well. You only need to keep __init__.py for the deprecated connected_components function.

This comment has been minimized.

@lesteve

lesteve Jun 2, 2017

Member

Or you could just add a test file with this content:

from sklearn.utils.sparsetools import connected_components

Your call

This comment has been minimized.

@naoyak

naoyak Jun 2, 2017

Contributor

Do we need this line in utils/setup.py?
#8854 (comment)
config.add_subpackage('sparsetools')

Apparently the lack of a setup.py and tests folder causes this test to fail..

I don't understand the implication of registering a subpackage with add_subpackage() - is this just for setting up the testrunner, or does it affect the ability to import submodules from sklearn?

This comment has been minimized.

@lesteve

lesteve Jun 2, 2017

Member

Apparently the lack of a setup.py and tests folder causes this test to fail..

OK I did not know that. I think this is fine like this then.

Do we need this line in utils/setup.py?
#8854 (comment)
config.add_subpackage('sparsetools')

I think so. One way to check would be to do python setup.py sdist and make sure that the folder sklearn.utils.sparsetools is in the sdist.

This comment has been minimized.

@naoyak

naoyak Jun 2, 2017

Contributor

got it. well, a bit annoying to require those placeholders but this seems to work.

@lesteve

This comment has been minimized.

Member

lesteve commented Jun 2, 2017

LGTM, I just pushed a small change to keep the signature of the original wrapper.

@@ -415,7 +415,7 @@ def logsumexp(arr, *args, **kwargs):
>>> logsumexp(a)
9.4586297444267107
"""
return scipy_logsumexp(arr, *args, **kwargs)
return scipy_logsumexp(arr, axis=0)

This comment has been minimized.

@naoyak

naoyak Jun 2, 2017

Contributor

oops ;)

@naoyak

This comment has been minimized.

Contributor

naoyak commented Jun 2, 2017

Thanks for the reviews - good to merge and move on to #8874?

@lesteve

This comment has been minimized.

Member

lesteve commented Jun 2, 2017

LGTM, merging, thanks a lot!

@lesteve lesteve merged commit bd0fc23 into scikit-learn:master Jun 2, 2017

4 of 5 checks passed

codecov/patch 94.17% of diff hit (target 94.88%)
Details
ci/circleci Your tests passed on CircleCI!
Details
codecov/project 95.94% (+1.05%) compared to a150d93
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@naoyak naoyak deleted the naoyak:scipy-deps branch Jun 3, 2017

Sundrique added a commit to Sundrique/scikit-learn that referenced this pull request Jun 14, 2017

[MRG+1] MAINT drop SciPy < 0.13 (scikit-learn#8854)
Remove sklearn.utils.fixes functions that are not needed for scipy >= 0.13 and keep deprecated wrappers in other modules.

dmohns added a commit to dmohns/scikit-learn that referenced this pull request Aug 7, 2017

[MRG+1] MAINT drop SciPy < 0.13 (scikit-learn#8854)
Remove sklearn.utils.fixes functions that are not needed for scipy >= 0.13 and keep deprecated wrappers in other modules.

dmohns added a commit to dmohns/scikit-learn that referenced this pull request Aug 7, 2017

[MRG+1] MAINT drop SciPy < 0.13 (scikit-learn#8854)
Remove sklearn.utils.fixes functions that are not needed for scipy >= 0.13 and keep deprecated wrappers in other modules.

NelleV added a commit to NelleV/scikit-learn that referenced this pull request Aug 11, 2017

[MRG+1] MAINT drop SciPy < 0.13 (scikit-learn#8854)
Remove sklearn.utils.fixes functions that are not needed for scipy >= 0.13 and keep deprecated wrappers in other modules.

paulha added a commit to paulha/scikit-learn that referenced this pull request Aug 19, 2017

[MRG+1] MAINT drop SciPy < 0.13 (scikit-learn#8854)
Remove sklearn.utils.fixes functions that are not needed for scipy >= 0.13 and keep deprecated wrappers in other modules.

AishwaryaRK added a commit to AishwaryaRK/scikit-learn that referenced this pull request Aug 29, 2017

[MRG+1] MAINT drop SciPy < 0.13 (scikit-learn#8854)
Remove sklearn.utils.fixes functions that are not needed for scipy >= 0.13 and keep deprecated wrappers in other modules.

maskani-moh added a commit to maskani-moh/scikit-learn that referenced this pull request Nov 15, 2017

[MRG+1] MAINT drop SciPy < 0.13 (scikit-learn#8854)
Remove sklearn.utils.fixes functions that are not needed for scipy >= 0.13 and keep deprecated wrappers in other modules.

jwjohnson314 pushed a commit to jwjohnson314/scikit-learn that referenced this pull request Dec 18, 2017

[MRG+1] MAINT drop SciPy < 0.13 (scikit-learn#8854)
Remove sklearn.utils.fixes functions that are not needed for scipy >= 0.13 and keep deprecated wrappers in other modules.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment