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

WIP: Reduce the default tolerance in threshold_li #3622

Merged
merged 2 commits into from
Mar 4, 2019

Conversation

jni
Copy link
Member

@jni jni commented Dec 22, 2018

Fixes #3605

In that issue, I found that a tolerance of range / 2**15 was
sufficient. However, I don't know whether this is generally true, so an
extra factor of 2 is probably a good margin, and gets us to a nice
round number.

Still to do:

  • Find an appropriate test case. I might end up using pooch/external data for this, so that I can use the example image from that issue.
  • Update the release notes both for dev and 0.14.1.
  • Write a dev gallery example about threshold_li. Possibly after merging attribute_operators.

Checklist

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

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.
  • Consider backporting the PR with @meeseeksdev backport to v0.14.x

Fixes scikit-image#3605

In that issue, I found that a tolerance of `range / 2**15` was
sufficient. However, the only thing that guarantees convergence is the
smallest intensity difference in the image (it's impossible for the
update to change if the threshold doesn't change). I've set this as the
new default, as the only downside is that it might take a long time to
converge, but that seems preferable to finding an incorrect threshold.
@jni
Copy link
Member Author

jni commented Dec 25, 2018

A small update: while I was playing around with the test image, I indeed found that a factor of 2 over the empirically found limit above was insufficient to ensure robustness. After I sampled pixels from the test image in #3605 to get a 1024x1024 image, range / 2**16 still got stuck in the low-gradient area. Part of the problem was that the image has a range greater than 8, but 98% of the pixels are in a much smaller range of less than 1. So the image range is inflated by outliers. But I think this is a general problem, regardless of the number of outliers.

In the end, I found a threshold that I think works in all cases: the tolerance should be strictly less than the smallest difference between two consecutive intensity values. I've made that (half the smallest gap) the default. I think in some cases you might end up making more updates than if you just brute-forced the threshold, but at least it will always be correct (and on average it should be much faster).

There's a chance that we could dramatically speed this up by sorting the intensities and only updating the foreground and background means incrementally, but I leave that for a later PR.

The rest of my to-do list remains to be done, just wanted to update with the current status.

Copy link
Member

@soupault soupault left a comment

Choose a reason for hiding this comment

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

Thanks Juan! Kind reminder about the other bullets.

@soupault soupault modified the milestones: 0.14.3, 0.15 Feb 28, 2019
@hmaarrfk hmaarrfk merged commit 42ff04c into scikit-image:master Mar 4, 2019
@lumberbot-app
Copy link

lumberbot-app bot commented Mar 4, 2019

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

  1. Checkout backport branch and update it.
$ git checkout v0.14.x
$ git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
$ git cherry-pick -m1 42ff04c884b84e69ec66ce6e56cd380c73e0f6e9
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
$ git commit -am 'Backport PR #3622: WIP: Reduce the default tolerance in threshold_li'
  1. Push to a named branch :
git push YOURFORK v0.14.x:auto-backport-of-pr-3622-on-v0.14.x
  1. Create a PR against branch v0.14.x, I would have named this PR:

"Backport PR #3622 on branch v0.14.x"

And apply the correct labels and milestones.

Congratulation you did some good work ! Hopefully your backport PR will be tested by the continuous integration and merged soon!

If these instruction are inaccurate, feel free to suggest an improvement.

@lumberbot-app lumberbot-app bot added the Still Needs Manual Backport MrMeeseeks-managed label label Mar 4, 2019
jni added a commit to jni/scikit-image that referenced this pull request Mar 4, 2019
* Reduce the default tolerance in threshold_li

Fixes scikit-image#3605

In that issue, I found that a tolerance of `range / 2**15` was
sufficient. However, the only thing that guarantees convergence is the
smallest intensity difference in the image (it's impossible for the
update to change if the threshold doesn't change). I've set this as the
new default, as the only downside is that it might take a long time to
converge, but that seems preferable to finding an incorrect threshold.

* Fix documentation of tolerance in threshold_li
hmaarrfk pushed a commit that referenced this pull request Mar 5, 2019
* Reduce default tolerance in threshold_li (#3622)

* Reduce the default tolerance in threshold_li

Fixes #3605

In that issue, I found that a tolerance of `range / 2**15` was
sufficient. However, the only thing that guarantees convergence is the
smallest intensity difference in the image (it's impossible for the
update to change if the threshold doesn't change). I've set this as the
new default, as the only downside is that it might take a long time to
converge, but that seems preferable to finding an incorrect threshold.

* Fix documentation of tolerance in threshold_li

* Backport Python3 float division in thresholding
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants