8000 Backport #3447: Fix morphology.local_maxima for input with any dimension < 3 by stefanv · Pull Request #3545 · scikit-image/scikit-image · GitHub
[go: up one dir, main page]
Skip to content

Backport #3447: Fix morphology.local_maxima for input with any dimension < 3 #3545

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
Closed
wants to merge 1 commit into from
Closed

Backport #3447: Fix morphology.local_maxima for input with any dimension < 3 #3545

wants to merge 1 commit into from

Conversation

stefanv
Copy link
Member
@stefanv stefanv commented Nov 13, 2018
  • Fix local_maxima for input with dimensions < 3

local_maxima (and local_minima) failed with an ValueError for images
with at least 1 dimension with fewer than 3 elements. This was due to
the implementation which applies a structuring element with exactly 3
elements in each dimension to the image in question. This behavior was
only visible if allow_borders was false, otherwise the image was
padded internally.
This fix makes these functions work for the mentioned edge cases by
testing the image shape at the appropriate time.

  • Fix output of local_maxima for empty arrays

The old output for empty arrays if indices was true didn't correspond
to the output for non-empty arrays and the description in the docstring.
This ensures that the output will always be a tuple of empty arrays.

If indices is false, the returned empty array now machtes the shape of
image as well.

  • Show warning if no maxima are possible

which is the case for dimensions with fewer than 3 samples. Because
local_minima wraps local_maxima the warning can only set the correct
stacklevel for one of these functions. The latter was chosen because it
stands to reason that it is / will be used more frequently.

If may be possible to suppress and re-raise the warning with the context
manager from the warnings module but that would introduce non-thread-
safe code and introduce perhaps unnecessary complexity.

  • Point user to allow_borders in warning message

Description

[Tell us about your new feature, improvement, or fix! If relevant, please supplement the PR with images.]

Checklist

[It's fine to submit PRs which are a work in progress! But before they are merged, all PRs should provide:]

[For detailed information on these and other aspects see scikit-image contribution guidelines]

References

[If this is a bug-fix or enhancement, it closes issue # ]
[If this is a new feature, it implements the following paper: ]

For reviewers

(Don't remove the checklist below.)

  • Check that the PR title is short, concise, and will make sense 1 year
    later.
  • Check that new functions are impor 8000 ted in corresponding __init__.py.
  • Check that new features, API changes, and deprecations are mentioned in
    doc/release/release_dev.rst.
  • Consider backporting the PR with @meeseeksdev backport to v0.14.x

@lagru @stefanv
Fix morphology.local_maxima for input with any dimension < 3 (scikit-…
6dc8b0e 
…image#3447)

* Fix local_maxima for input with dimensions < 3

local_maxima (and local_minima) failed with an ValueError for images
with at least 1 dimension with fewer than 3 elements. This was due to
the implementation which applies a structuring element with exactly 3
elements in each dimension to the image in question. This behavior was
only visible if `allow_borders` was false, otherwise the image was
padded internally.
This fix makes these functions work for the mentioned edge cases by
testing the image shape at the appropriate time.

* Fix output of local_maxima for empty arrays

The old output for empty arrays if `indices` was true didn't correspond
to the output for non-empty arrays and the description in the docstring.
This ensures that the output will always be a tuple of empty arrays.

If `indices` is false, the returned empty array now machtes the shape of
`image` as well.

* Show warning if no maxima are possible

which is the case for dimensions with fewer than 3 samples. Because
`local_minima` wraps `local_maxima` the warning can only set the correct
stacklevel for one of these functions. The latter was chosen because it
stands to reason that it is / will be used more frequently.

If may be possible to suppress and re-raise the warning with the context
manager from the warnings module but that would introduce non-thread-
safe code and introduce perhaps unnecessary complexity.

* Point user to allow_borders in warning message
@pep8speaks
Copy link

Hello @stefanv! Thanks for submitting the PR.

Line 291:1: E302 expected 2 blank lines, found 1

@stefanv stefanv mentioned this pull request Nov 13, 2018
Merged
9 tasks
@jni
Copy link
Member
jni commented Nov 13, 2018

Did you miss some commits or did we forget to backport a prerequisite PR? These changes clearly don't make sense on their own. See:
https://github.com/scikit-image/scikit-image/pull/3545/files#diff-1b49cb33a2ea212f0677bfe8ba6f3fcdR302

definition only has image and selem, but this PR appears to add many more.

@stefanv
Copy link
Member Author
stefanv commented Nov 13, 2018

I guess there must be other PRs that were missed; I wasn't tracking.

@stefanv stefanv closed this Nov 14, 2018
@lagru
Copy link
Member
lagru commented Nov 14, 2018

Did you miss some commits or did we forget to backport a prerequisite PR?

Yeah, you missed #3022 which introduces the rewrite of local_maxima. I'd be willing to help. They way I understand it I'd just need to do a PR with the commits in question against scikit-image:v0.14.x. Correct?

@jni
Copy link
Member
jni commented Nov 14, 2018

Ooops! I can see that we even tried the auto backport and then totally dropped the ball when it failed. Yes @lagru that's indeed what's required. Maybe you can pile these on as well onto the same PR? =)

@lagru
Copy link
Member
lagru commented Nov 14, 2018 •
edited
Loading

@jni Happy to do so. However maybe you can give me some tips so I do this the right way from the start:

My first impulse is to cherry-pick the commits in these two PRs (sans merge commits?) onto a new branch (e.g. backport_v0.14.x/3022_3447) which is based on v0.14.x. But it looks like I'd have to do so for every commit in #3022 of which there are 35. Is there a better less error-prone way?

@jni
Copy link
Member
jni commented Nov 14, 2018

git cherry-pick start_commit..end_commit =)

@lagru
Copy link
Member
lagru commented Nov 14, 2018

Thanks! Why didn't I think of that? 🙈

@lagru lagru mentioned this pull request Nov 14, 2018
Merged
9 tasks
@jni
Copy link
Member
jni commented Nov 15, 2018

@lagru because git man-pages are appallingly bad. =D I highly recommend tldr:

 $ tldr git-cherry-pick
git cherry-pick
Apply the changes introduced by existing commits to the current branch.
To apply changes to another branch, first use git-checkout to switch to the desired branch.

 - Apply a commit to the current branch:
   git cherry-pick {{commit}}

 - Apply a range of commits to the current branch (see also 
   git rebase --onto
):
   git cherry-pick {{start_commit}}~..{{end_commit}}

 - Apply multiple (non-sequential) commits to the current branch:
   git cherry-pick {{commit_1}} {{commit_2}}

Incidentally, it looks like my advice was slightly wrong... There should have been a ~ after the first commit. Just tested it out and indeed it needs to be there. Looks like it's a half-open interval on the start side? Weird. Can you double check that you got all the commits you meant to in #3546? Or did you figure this out on your own in the end?

@lagru
Copy link
Member
lagru commented Nov 15, 2018

@jni Yes, The PR has all commits except for the merge-commits.
I was slightly surprised that I got a merge conflict for the second cherry-picked commit when there shouldn't have been one. So I figured its an half-open interval and just specified the parent commit. 😄

I highly recommend tldr

Thank you! This is gold!

@stefanv stefanv deleted the backport_v0.14.x/3447 branch November 28, 2018 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@stefanv @pep8speaks @jni @lagru
0