[MRG+2] MAINT: no longer backport graph_laplacian, use scipy one#9077
[MRG+2] MAINT: no longer backport graph_laplacian, use scipy one#9077lesteve merged 3 commits intoscikit-learn:masterfrom
Conversation
sklearn/utils/graph.py
Outdated
|
|
||
| import numpy as np | ||
| from scipy import sparse | ||
| from scipy.sparse.csgraph import laplacian as graph_laplacian |
There was a problem hiding this comment.
pyflake raises F401: imported but unused on this line.
However, it is used from other files, just not from this one.
I am inclined to add #noqa and keep providing it this way.
Alternative 2: fix our codebase to directly import from scipy, and provide a thin function here that raises deprecation warnings (in case users are calling this directly.)
after some thought, alternative 2 sounds better. But it was healthy to have a run of CI with this simple commit.
|
|
||
|
|
||
| @ignore_warnings(category=DeprecationWarning) | ||
| def test_graph_laplacian(): |
There was a problem hiding this comment.
maybe this test (and this whole file) can go
There was a problem hiding this comment.
sure, the very same test is done in scipy directly https://github.com/scipy/scipy/blob/master/scipy/sparse/csgraph/tests/test_graph_laplacian.py#L61
however, maybe we might want to add tests for the extra math properties of graph Laplacians here (e.g. for a fully connected graph, the first eigenvector is constant) in this file
There was a problem hiding this comment.
to me, the point of the test currently is simply to check that importing sklearn.utils.graph_laplacian is not broken, because we are still supporting it for two releases. When that import point will go away, the test will fail, reminding us to remove it. I could replace it by a simpler guard test that just imports, checks for deprecation warning, and returns, but this is lazier.
Actual functionality tests for math properties should from now on go into scipy, IMO, unless we find a bug and backport it again.
|
LGTM |
|
LGTM |
|
I pushed a tiny change in the whats_new, merging, thanks a lot! |
Reference Issue: None
What does this implement/fix? Explain your changes.
Clean-up: our minimum supported scipy (0.13) has a compatible implementation of graph laplacian, so we no longer need to keep a copy in our repo.
Any other comments?
The upstream implementation might have a couple of optimizations compared to ours, but nothing breaking. I kept our test in utils, to have it run once on CI; then I will remove it. I ran the full test suite with scipy 0.13.3 on my machine.
Ping @glemaitre, @jmargeta; helps the debugging scope of #9062