8000 Make boundary mode names consistent and improve their documentation by grlee77 · Pull Request #6364 · scikit-image/scikit-image · GitHub
[go: up one dir, main page]
Skip to content

Make boundary mode names consistent and improve their documentation #6364

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 8 commits into
base: main
Choose a base branch
from
Draft

Make boundary mode names consistent and improve their documentation #6364

wants to merge 8 commits into from

Conversation

grlee77
Copy link
Contributor
@grlee77 grlee77 commented Apr 27, 2022

Description

This PR makes edge modes consistent as discussed previously in #5439 (comment).

closes #6345: cross-links to documentation illustrating the edge modes

I went with the scipy.ndimage mode names because those were more common across the library. The only exception that still usesnumpy.pad mode names is skimage.features.match_template as this function explicitly documents that the mode is passed directly to an internal numpy.pad call.

This PR is a good example of a case where deprecation isn't really feasible as the default of mode='reflect' exists both for numpy.pad and scipy.ndimage but the behavior is subtly different (scipy.ndimage repeats the edge value while numpy.pad's 'reflect' behaves like ndimage's 'mirror'). Thus I propose we only introduce this change for skimage2 and delay merging this PR until after 1.0.x has been branched off of main.

Summary of the change for existing skimage.transforms code:

  • If the default mode was being used, behavior remains the same
  • mode='wrap' and mode='constant' continue to behave the same
  • if mode='reflect' was being used it now behaves slightly differently (the edge value is duplicated). To get the old behavior, the user would have to pass mode='mirror' instead.
  • mode='symmetric' will now raise a RuntimeError and require replacement with mode='reflect' for equivalent behavior
  • mode='edge' will now raise a RuntimeError and require replacement with mode='nearest' for equivalent behavior

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 added 6 commits April 27, 2022 12:55
@grlee77
clarify that the mode used by skimage.feature.match_template is for n…
824a288 
…umpy.pad
@grlee77
update denoise_bilateral to use scipy.ndimage mode names
d86b125
@grlee77
change all skimage.transforms functions to use scipy.ndimage mode names
5684bef 
many names are the same, but default of 'reflect' for numpy.pad
corresponds to 'mirror' for ndimage
@grlee77
cross-link to scipy.ndimage documentation with detailed mode descript…
ed28221 
…ions
@grlee77
have skimage.transforms functions always convert 'constant'->'grid-co…
4b63878 
…nstant'

legacy Cython interpolation code in skimage behaves like 'grid-constant' in ndimage.
this avoids scipy.ndimage warnings recommending 'grid-constant' over 'constant'.
@grlee77
fix scipy intersphinx cross-links
21b0ecb
@grlee77 grlee77 added 📄 type: Documentation Updates, fixes and additions to documentation 📜 type: API Involves API change(s) labels Apr 27, 2022
@grlee77 grlee77 added this to the skimage2 milestone Apr 27, 2022
@grlee77
Copy link
Contributor Author
grlee77 commented Apr 27, 2022

may need to revert 4b63878 unless we update to SciPy >=1.6.0

@grlee77 grlee77 marked this pull request as draft April 27, 2022 21:23
alexdesiqueira
alexdesiqueira reviewed Apr 27, 2022
View reviewed changes
@grlee77
Update skimage/_shared/filters.py
374c764 
Co-authored-by: Alexandre de Siqueira <alex.desiqueira@igdore.org>
@alexdesiqueira
Copy link
Member

@grlee77 I think this is good to go, thanks! I'll ask @stefanv input on this one. Is there anything else you'd like to add?

@stefanv
Copy link
Member
stefanv commented Jun 13, 2022

Updating to scipy 1.6 feels like the more future-proof solution. We don't really have the bandwidth to baby-sit older versions of SciPy, so I am +1 on doing that.

@stefanv
Copy link
Member
stefanv commented Jun 13, 2022

Agreed that changing interpolation modes to be consistent may need to wait for skimage2.

@grlee77 grlee77 mentioned this pull request Oct 16, 2022
Open
stefanv
stefanv reviewed Oct 17, 2022
View reviewed changes
Copy link
Member
@stefanv stefanv left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Is edge and nearest the same? I'd expect them to differ in some cases.

@stefanv stefanv added this to skimage2 Mar 1, 2025
@stefanv stefanv moved this to To Do in skimage2 Mar 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stefanv stefanv stefanv left review comments

@alexdesiqueira alexdesiqueira alexdesiqueira left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
✋ Wait for skimage2 📄 type: Documentation Updates, fixes and additions to documentation 📜 type: API Involves API change(s)
Projects
skimage2
Status: To Do
Milestone
2.0
Development

Successfully merging this pull request may close these issues.

Edge modes are either undocumented, or that documentation is hard to find
3 participants
@grlee77 @alexdesiqueira @stefanv
0