Move future.graph to skimage.graph
#6674
Conversation
I opted to make all moved submodules private as I expect the contained public functions to be exposed in `skimage.graph`. Also, I removed the duplication of `cut_normalized` in `ncut`.
|
How about an error upon import of |
|
Great, call. Seems like an obvious thing to do now. 馃檲 I'm on it. |
I'm optimistically using v0.20 here. If this is not merged in time, I'll update it accordingly for v0.21.
| with pytest.raises(ModuleNotFoundError, match=error_msg): | ||
| import skimage.future.graph | ||
|
|
||
| with pytest.raises(ModuleNotFoundError, match=error_msg): | ||
| from skimage.future import graph |
There was a problem hiding this comment.
These two tests test the same import?
There was a problem hiding this comment.
Yes, the module and the contained raise statement is run regardless of the import statement. This even applies for the from skimage.future.graph import x case! You can see the test's passing in the CI for the install from sdist job. Other jobs fail because they trigger the raise statement during doctest collection...
I haven't yet figured out the best approach to resolve this. We could likely use pytest_ignore_collect but I'd like to avoid that for a temporary patch.
There was a problem hiding this comment.
Using a warning instead would work. But the migration message doesn't appear as nicely at the end of the traceback..
There was a problem hiding this comment.
Interesting. Adding the following to our conftest.py
def pytest_ignore_collect(collection_path, path, config):
return str(collection_path).endswith("skimage/future/graph.py")works to ignore the ModuleNotFoundError in graph.py . However, it then stumbles across
scikit-image/skimage/io/_plugins/gdal_plugin.py
Lines 6 to 8 in 8233150
I'm lost why that isn't the case if I remove pytest_ignore_collect. 馃 Do we have some magic somewhere that ignores that file during doctest collection?
There was a problem hiding this comment.
Aha:
scikit-image/skimage/conftest.py
Line 4 in 8233150
| with pytest.raises(ModuleNotFoundError, match=error_msg): | ||
| from skimage.future.graph import rag |
There was a problem hiding this comment.
No way the above tests pass and this one fails.
There was a problem hiding this comment.
But they do. See #6674 (comment). 馃槄
Co-authored-by: Stefan van der Walt <sjvdwalt@gmail.com>
lagru
force-pushed
the
move-future-graph
branch
3 times, most recently
from
9f2f7c6
to
9dc056e
Compare
Not doing this will trigger the ModuleNotFoundError in skimage/future/graph/__init__.py. The ignore_collect entry doesn't seem to work if `graph` is a module in `skimage.future`; it works when we turn it into a package.
lagru
force-pushed
the
move-future-graph
branch
from
9dc056e
to
4a36c0f
Compare
|
Finally green. As both of you approved I'm going ahead and merge this now. Thanks for the fast reviews! |
Description
Closes #3105.
Not sure if we want some outstanding changes to the API but I figured, this way we could get the discussion started. I opted to make all moved submodules private as I expect the contained public functions to be exposed in
skimage.graph. Also, I removed the duplication ofcut_normalizedinncut; unless there is a specific reason to keep both functions, I'd rather have "one-- and preferably only one --obvious way to [call] it". 馃槈Checklist
./doc/examples(new features only)./benchmarks, if your changes aren't covered by anexisting benchmark
For reviewers
later.
__init__.py.doc/release/release_dev.rst.example, to backport to v0.19.x after merging, add the following in a PR
comment:
@meeseeksdev backport to v0.19.xrun-benchmarklabel. To rerun, the labelcan be removed and then added again. The benchmark output can be checked in
the "Actions" tab.