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.

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

Fixed off-by one error in pixel bins in hough line transform #5183

Merged
merged 2 commits into from
Jan 15, 2021
Merged

Fixed off-by one error in pixel bins in hough line transform #5183

merged 2 commits into from
Jan 15, 2021

Conversation

madphysicist
Copy link
Contributor

Description

Assuming that the goal is to have step in rho be 1px, the new formulation is correct. There is no issue open, but that can be done. This simplifies the computation by starting with the offset first, then doubling and adding one. The original code used double without the plus one.

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.

@madphysicist madphysicist marked this pull request as draft January 13, 2021 19:25
@madphysicist madphysicist marked this pull request as ready for review January 13, 2021 20:52
jni
jni approved these changes Jan 13, 2021
View reviewed changes
Copy link
Member

@jni jni left a comment

Choose a reason for hiding this comment

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

This LGTM, thanks @madphysicist! The single CI failure appears to be a stalled VM, nothing to do with the changes.

@jni jni added the 🩹 type: Bug fix Fixes unexpected or incorrect behavior label Jan 13, 2021
@jni jni changed the title Fixed off-by one error in pixel bins Fixed off-by one error in pixel bins in hough line transform Jan 13, 2021
@madphysicist madphysicist mentioned this pull request Jan 14, 2021
Merged
rfezzani
rfezzani approved these changes Jan 14, 2021
View reviewed changes
Copy link
Member

@rfezzani rfezzani left a comment

Choose a reason for hiding this comment

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

Excellent! Good catch @madphysicist 😉

@rfezzani
Copy link
Member

Following @madphysicist advise #5182 (comment), please don't merge before #5182.

@madphysicist
Fixed off-by one error in pixel bins
6626d65 
Assuming that the goal is to have step in rho be 1px, the new formulation is correct.
@madphysicist
Fixed tests
b2b6e04 
Test values should definitely be integers now
@madphysicist
Copy link
Contributor Author

Took a couple of tries, but looks like I rebased correctly now.

@jni jni merged commit 4f93a9f into scikit-image:master Jan 15, 2021
@jni
Copy link
Member

jni commented Jan 15, 2021

🙏

@madphysicist madphysicist deleted the patch-1 branch January 15, 2021 14:19
@madphysicist
Copy link
Contributor Author

All these PRs happened because I had an image with a single perfect straight line that I needed to identify. Took me a whole day to realize that I had accidentally flipped the < operator to > when I did thresholding. In the meantime, I got pretty familiar with the Hough functions here :)

@jni jni mentioned this pull request Jan 18, 2021
Closed
rfezzani added a commit that referenced this pull request Jan 22, 2021
@grlee77 @crisluengo
@rfezzani @jni @mkcor @madphysicist @sciunto @mark-boer @stefanv @charlielito
Simplify resize implementation using new SciPy 1.6 zoom option (#5173)
d2fb610 
* resize using scipy.ndimage.zoom

* Clip negative values in output of RGB to HED conversion. Fixes #5164

* Fixed stain separation tests.

- Round trips start with stains, such that the roundtrip does not require
negative values.
- Using a blue+red+orange stain combination, the test previously used
a 2-stain combination that made it impossible to do a correct roundtrip.

* Improve IHC stain separation example.

* BENCH: add benchmarks for resize and rescale

* move mode conversion code to _shared.utils.py

use grid-constant and grid-wrap for SciPy >= 1.6.0

closes gh-5064

* separate grid mode conversion utility

* Update doc/examples/color_exposure/plot_ihc_color_separation.py

Improvement to the IHC example.

Co-authored-by: Riadh Fezzani <rfezzani@gmail.com>

* add TODO for removing legacy code paths

* avoid pep8 warning in benchmark_interpolation.py

* keep same ValueError on unrecognized modes

* test_rank.py: fetch reference data once into a dictionary of arrays (#5175)

This should avoid stochastic test failures that have been occuring on GitHub Actions

* Add normalized mutual information metric (#5158)

* Add normalized mutual information metric

* Add tests for NMI

* Add NMI to metrics init

* Fix error message for incorrect target shape

* Add 3D NMI test

* properly add nmi to metrics init

* _true, _test -> 0, 1

* ravel -> reshape(-1)

* Apply suggestions from @mkcor's code review

Co-authored-by: Marianne Corvellec <marianne.corvellec@ens-lyon.org>

* Fix warning and exception messages

Co-authored-by: Marianne Corvellec <marianne.corvellec@ens-lyon.org>

* segment Other section of TODO by date

based on NEP-29 24 month window for dropping upstream package version

* Fix bug in optical flow code and examples

mode 'nearest' is a scipy.ndimage mode, but warp only accepts the numpy.pad equivalent of 'edge'

* fix additional cases of warp with 'nearest'->'edge'

* DOC: Fixup to linspace in example (#5181)

Right now the data is not sampled every half degree. Also, -pi/2 to pi/2.

* Minor fixups to Hough line transform code and examples (#5182)

* Correction to linspace call in hough_line default

`theta = np.pi / 2 - np.arange(180) / 180.0 * np.pi` done correctly below.

* BUG: Fixup to image orientation in example

* DOC: Tiny typo

* Changed range to conform to documentation

The original was [pi/2, -pi/2), not [-pi/2, pi/2). Updated wording in docs.

* Fixed x-bounds of hough image in example

* Fixed long line

* Fixes to tests

Number of lines in checkerboard, longer line in peak ordering assertion, updated angle

* Accidentally modified the wrong value.

* Updated example to include three lines

As requested in review

* Fixed off-by one error in pixel bins in hough line transform (#5183)

* Added 1/2 pixel bounds to extent of displayed images (#5184)

* fast, non-Cython implementation for correlate_sparse (#5171)

* Replace correlate_sparse Cython version with a simple slicing-based variant in _mean_std

This slicing variant has two benefits:
1.) profiled 30-40% faster on 2d and 3d cases I tested with
2.) can immediately be used by other array backends such as CuPy

* remove intermediate padded_sq variable to reduce memory footprint

* Update skimage/filters/_sparse.py

remove duplicate declarations

Co-authored-by: Riadh Fezzani <rfezzani@gmail.com>

* move _to_np_mode and _to_ndimage_mode to _shared.utils.py

* add comment about corner_index needing to be present

* raise RuntimeError on missing corner index

* update code style based on reviewer comments

Co-authored-by: Riadh Fezzani <rfezzani@gmail.com>

* Add release step on github to RELEASE.txt (See #5185) (#5187)

Co-authored-by: Juan Nunez-Iglesias <juan.nunez-iglesias@monash.edu>

* Prevent integer overflow in EllipseModel (#5179)

* Change dtype of CircleModel and EllipseModel to float, to avoid integer overflows

* remove failing test and add code review notes

* Apply suggestions from code review

Co-authored-by: Stefan van der Walt <sjvdwalt@gmail.com>

* Remove unused pytest import

Co-authored-by: Stefan van der Walt <sjvdwalt@gmail.com>

Co-authored-by: Stefan van der Walt <sjvdwalt@gmail.com>
Co-authored-by: Juan Nunez-Iglesias <juan.nunez-iglesias@monash.edu>

* Add saturation parameter to color.label2rgb (#5156)

* Remove reference to opencv in threshold_local documentation (#5191)

Co-authored-by: Marianne Corvellec <marianne.corvellec@ens-lyon.org>
Co-authored-by: Riadh <r.fezzani@vitadx.com>

* resize using scipy.ndimage.zoom

* BENCH: add benchmarks for resize and rescale

* move mode conversion code to _shared.utils.py

use grid-constant and grid-wrap for SciPy >= 1.6.0

closes gh-5064

* separate grid mode conversion utility

* add TODO for removing legacy code paths

* avoid pep8 warning in benchmark_interpolation.py

* keep same ValueError on unrecognized modes

* segment Other section of TODO by date

based on NEP-29 24 month window for dropping upstream package version

* Fix bug in optical flow code and examples

mode 'nearest' is a scipy.ndimage mode, but warp only accepts the numpy.pad equivalent of 'edge'

* fix additional cases of warp with 'nearest'->'edge'

* Apply suggestions from code review

Co-authored-by: Riadh Fezzani <rfezzani@gmail.com>

Co-authored-by: Cris Luengo <cris.l.luengo@gmail.com>
Co-authored-by: Cris Luengo <crisluengo@users.noreply.github.com>
Co-authored-by: Riadh Fezzani <rfezzani@gmail.com>
Co-authored-by: Alexandre de Siqueira <alex.desiqueira@igdore.org>
Co-authored-by: Juan Nunez-Iglesias <juan.nunez-iglesias@monash.edu>
Co-authored-by: Marianne Corvellec <marianne.corvellec@ens-lyon.org>
Co-authored-by: Joseph Fox-Rabinovitz <madphysicist@users.noreply.github.com>
Co-authored-by: François Boulogne <fboulogne@sciunto.org>
Co-authored-by: Mark Boer <m.h.boer.2@gmail.com>
Co-authored-by: Stefan van der Walt <sjvdwalt@gmail.com>
Co-authored-by: Carlos Andrés Álvarez Restrepo <charlie_cha@outlook.com>
Co-authored-by: Riadh <r.fezzani@vitadx.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jni jni jni approved these changes

@rfezzani rfezzani rfezzani approved these changes

Assignees
No one assigned
Labels
🩹 type: Bug fix Fixes unexpected or incorrect behavior
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@madphysicist @rfezzani @jni