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

remove extraneous function in createluts.py (and move mc_meta reference code) #6294

Merged
merged 3 commits into from Mar 30, 2022

Conversation

grlee77
Copy link
Contributor

@grlee77 grlee77 commented Mar 21, 2022

Description

The first commit avoids the flake8 warning about an undefined variable luts reported in mc_meta/createluts.py (see #4163).

The remaining commits move the whole mc_meta folder out of the skimage package path so that it will not be distributed with scikit-image. It appears to just be there for developer reference and is not otherwise used. I chose to place it in tools/precompute which is the path used for scripts precomputing attributes for the moments computations in #6188 as well as for the proposed morphology decompositions in #6150. I think it is nice to have the scripts for cases like these available in the repository for developer reference, but they do not need to be distributed to end users.

Checklist

  • Docstrings for all functions
  • Gallery example in ./doc/examples (new features only)
  • Benchmark in ./benchmarks, if your changes aren't covered by an
    existing benchmark
  • Unit tests
  • Clean style in the spirit of PEP8
  • Descriptive commit messages (see below)

For reviewers

  • Check that the PR title is short, concise, and will make sense 1 year
    later.
  • Check that new functions are imported in corresponding __init__.py.
  • Check that new features, API changes, and deprecations are mentioned in
    doc/release/release_dev.rst.
  • There is a bot to help automate backporting a PR to an older branch. For
    example, to backport to v0.19.x after merging, add the following in a PR
    comment: @meeseeksdev backport to v0.19.x
  • To run benchmarks on a PR, add the run-benchmark label. To rerun, the label
    can be removed and then added again. The benchmark output can be checked in
    the "Actions" tab.

@grlee77 grlee77 added the 🔧 type: Maintenance Refactoring and maintenance of internals label Mar 21, 2022
@grlee77 grlee77 changed the title remove extraneous function in createluts.py remove extraneous function in createluts.py (and move mc_meta reference code) Mar 21, 2022
Copy link
Member

@mkcor mkcor left a comment

Choose a reason for hiding this comment

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

Great! tools/precompute is the right place, consistently with these other PRs you referred to 👍

@lagru lagru merged commit 1af0071 into scikit-image:main Mar 30, 2022
@lagru
Copy link
Member

lagru commented Mar 30, 2022

Merging as I guess the doc timeout is unrelated. Thanks to both of you. Unless I'm missing something, I don't think this PR should close the original issue #4163 as only one of the undefined variables is addressed.

./doc/ext/notebook_doc.py:1:1: F822 undefined name 'python_to_notebook' in __all__
__all__ = ['python_to_notebook', 'Notebook']

is still found by flake8.

@lagru
Copy link
Member

lagru commented Mar 30, 2022

See #6307. In retrospect, I could probably have pushed that change here.

alexdesiqueira pushed a commit to alexdesiqueira/scikit-image that referenced this pull request May 24, 2022
…ce code) (scikit-image#6294)

* remove extraneous function in createluts.py

avoids flake8 warning about undefined  variable

* move mc_meta reference script out of the skimage namespace

update comment at top of _marching_cubes_lewiner_luts.py

* skip of mc_meta/visual_test.py no longer needed in conftest.py
@lagru lagru added this to the 0.20 milestone Oct 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔧 type: Maintenance Refactoring and maintenance of internals
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants