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

Audit our code import structure #3397

Open
1 of 5 tasks
jni opened this issue Sep 10, 2018 · 6 comments
Open
1 of 5 tasks

Audit our code import structure #3397

jni opened this issue Sep 10, 2018 · 6 comments
Labels
💬 Discussion 🔧 type: Maintenance Refactoring and maintenance of internals

Comments

@jni
Copy link
Member

jni commented Sep 10, 2018

Description

I think we have become a little too lackadaisical in how we reuse our code from different modules across modules. Things used across several modules (segmentation, morphology, etc) should probably be moved to "util".

Here is a user's traceback (from SO) after trying to import feature.greycomatrix:

Traceback (most recent call last):
  File "C:\Users\Martin Ma\Desktop\every day python\test\GLCM\main.py", line 80, in <module>
    from skimage.feature import greycomatrix
  File "E:\python\lib\site-packages\skimage\feature\__init__.py", line 9, in <module>
    from .peak import peak_local_max
  File "E:\python\lib\site-packages\skimage\feature\peak.py", line 3, in <module>
    from ..segmentation import relabel_sequential
  File "E:\python\lib\site-packages\skimage\segmentation\__init__.py", line 1, in <module>
    from .random_walker_segmentation import random_walker
  File "E:\python\lib\site-packages\skimage\segmentation\random_walker_segmentation.py", line 43, in <module>
    from ..filters import rank_order
  File "E:\python\lib\site-packages\skimage\filters\__init__.py", line 3, in <module>
    from .edges import (sobel, sobel_h, sobel_v,
  File "E:\python\lib\site-packages\skimage\filters\edges.py", line 17, in <module>
    from ..restoration.uft import laplacian
  File "E:\python\lib\site-packages\skimage\restoration\__init__.py", line 12, in <module>
    from .inpaint import inpaint_biharmonic
  File "E:\python\lib\site-packages\skimage\restoration\inpaint.py", line 9, in <module>
    from ..measure import label
  File "E:\python\lib\site-packages\skimage\measure\__init__.py", line 6, in <module>
    from ._regionprops import regionprops, perimeter
  File "E:\python\lib\site-packages\skimage\measure\_regionprops.py", line 645, in <module>
    _install_properties_docs()
  File "E:\python\lib\site-packages\skimage\measure\_regionprops.py", line 632, in _install_properties_docs
    prop_doc = _parse_docs()
  File "E:\python\lib\site-packages\skimage\measure\_regionprops.py", line 625, in _parse_docs
    doc, flags=re.DOTALL)
  File "E:\python\lib\re.py", line 229, in finditer
    return _compile(pattern, flags).finditer(string)
TypeError: expected string or bytes-like object

That just looks insane to me.

Way to reproduce

[If reporting a bug, please include the following important information:]

  • Code example
  • Relevant images (if any)
  • Operating system and version
  • Python version
  • scikit-image version (run skimage.__version__)
@soupault soupault added this to the 1.0 milestone Sep 10, 2018
@hmaarrfk
Copy link
Member

hmaarrfk commented Oct 2, 2018

While this doesn't seem important, I just ran into this issue when trying to import a support function warn from the internal _shared directory.

#3438

@stefanv
Copy link
Member

stefanv commented Oct 2, 2018

Currently, functionality lives where they best belong, and gets used from other submodules where appropriate. So, yes, this sometimes make these chains. But what do you prefer—that functions with clearly defined submodule homes live in a general utilities module instead?

@jni
Copy link
Member Author

jni commented Oct 3, 2018

@stefanv (a) but "best" is often debatable. Looking at our import graph could show where functionality might best be moved. In the traceback above, does the laplacian filter really need to live in restoration.uft? (b) things that are used elsewhere could indeed live in util or _shared and get imported in modules where we want them to be exposed publicly. Internally we would use util.

I honestly get your point — in my proposal (b), it might make our own code harder to follow if we're importing gaussian from util instead of filters — and I don't know what the best solution is, but I want this to be on our radar. Even "nice" tracebacks are intimidating to beginners. The traceback above is intimidating to me.

@stefanv
Copy link
Member

stefanv commented Oct 4, 2018 via email

@hmaarrfk
Copy link
Member

Maybe think of this:
https://github.com/bndr/pycycle

@grlee77 grlee77 added this to To Do in skimage2 API Apr 17, 2021
@grlee77 grlee77 removed this from To Do in skimage2 API Apr 17, 2021
@scikit-image scikit-image locked and limited conversation to collaborators Oct 18, 2021
@scikit-image scikit-image unlocked this conversation Mar 27, 2022
@grlee77
Copy link
Contributor

grlee77 commented Mar 27, 2022

moved over from deleted Discussion topic

rfezzani wrote

I opened #5994 to remove the circular imports that I detected using

grep -r "    from \.\." --include=*.py skimage/

on main branch I obtain

skimage/viewer/canvastools/painttool.py:    from ... import data
skimage/viewer/canvastools/painttool.py:    from ...viewer import ImageViewer
skimage/viewer/canvastools/linetool.py:    from ... import data
skimage/viewer/canvastools/linetool.py:    from ...viewer import ImageViewer
skimage/viewer/canvastools/recttool.py:    from ...viewer import ImageViewer
skimage/viewer/canvastools/recttool.py:    from ... import data
skimage/feature/_hog.py:        from .. import draw
skimage/io/_plugins/skivi.py:        from ... import io
skimage/io/_plugins/skivi.py:        from ... import io
skimage/data/__init__.py:    from ..io import imread
skimage/measure/_regionprops.py:        from ..morphology.convex_hull import convex_hull_image
skimage/measure/_label.py:    from ..morphology._util import _resolve_neighborhood
skimage/measure/_blur_effect.py:    from ..filters import sobel
skimage/transform/hough_transform.py:    from ..feature.peak import _prominent_peaks
skimage/transform/hough_transform.py:    from ..feature.peak import _prominent_peaks

and in the PR

skimage/morphology/tests/test_convex_hull.py:    from ...measure.tests.test_regionprops import SAMPLE as image
skimage/viewer/canvastools/painttool.py:    from ... import data
skimage/viewer/canvastools/painttool.py:    from ...viewer import ImageViewer
skimage/viewer/canvastools/linetool.py:    from ... import data
skimage/viewer/canvastools/linetool.py:    from ...viewer import ImageViewer
skimage/viewer/canvastools/recttool.py:    from ...viewer import ImageViewer
skimage/viewer/canvastools/recttool.py:    from ... import data
skimage/io/_plugins/skivi.py:        from ... import io
skimage/io/_plugins/skivi.py:        from ... import io
skimage/data/__init__.py:    from ..io import imread

I didn't addressed

@grlee77 grlee77 reopened this Mar 27, 2022
@grlee77 grlee77 added 🔧 type: Maintenance Refactoring and maintenance of internals 💬 Discussion labels Mar 27, 2022
@lagru lagru removed this from the 0.21 milestone Oct 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💬 Discussion 🔧 type: Maintenance Refactoring and maintenance of internals
Projects
None yet
Development

No branches or pull requests

7 participants