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

Improve histogram matching performance on unsigned integer data (resume #6209) #6354

Merged
merged 9 commits into from Apr 25, 2022

Conversation

grlee77
Copy link
Contributor

@grlee77 grlee77 commented Apr 24, 2022

Description

This PR adds the suggestions from #6209. Benchmark results show a large improvement for unsigned integer types as compared to floating point types. The improvement is > 10x at larger sizes.


[100.00%] ··· benchmark_exposure.MatchHistogramsSuite.time_match_histogram 
[100.00%] ··· ============== =============== ============= =============
              --                                     multichannel       
              ------------------------------ ---------------------------
                  shape           dtype          False          True    
              ============== =============== ============= =============
                 (64, 64)      numpy.uint8     54.8±0.4μs     149±2μs   
                 (64, 64)      numpy.uint32    56.0±0.9μs     154±1μs   
                 (64, 64)     numpy.float32     287±1μs       843±7μs   
                 (64, 64)     numpy.float64    285±0.8μs      827±20μs  
                (256, 256)     numpy.uint8      354±7μs     1.30±0.02ms 
                (256, 256)     numpy.uint32     411±5μs     1.64±0.01ms 
                (256, 256)    numpy.float32   4.25±0.03ms   13.2±0.04ms 
                (256, 256)    numpy.float64    4.40±0.1ms    13.4±0.1ms 
               (1024, 1024)    numpy.uint8    5.33±0.04ms    23.2±0.5ms 
               (1024, 1024)    numpy.uint32   6.35±0.05ms    31.3±0.6ms 
               (1024, 1024)   numpy.float32    87.0±0.2ms     268±3ms   
               (1024, 1024)   numpy.float64    96.2±0.8ms     288±7ms   
              ============== =============== ============= =============

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.

kwikwag and others added 6 commits April 24, 2022 14:56
`np.bincount` is preferable to `np.unique` for unsigned integer types (i.e. most image representations). This affords a 6-fold performance increase for these types of inputs.
@pep8speaks
Copy link

pep8speaks commented Apr 24, 2022

Hello @grlee77! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2022-04-25 12:19:54 UTC

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.

Thank you @grlee77, looks good 😉. Can you add a unitest to check the consistency of the result depending on input dtype?

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.

Thank you @grlee77!

@grlee77 grlee77 merged commit 7f22614 into scikit-image:main Apr 25, 2022
@grlee77
Copy link
Contributor Author

grlee77 commented Apr 25, 2022

Thanks for adding this fast uint code path @kwikwag

alexdesiqueira pushed a commit to alexdesiqueira/scikit-image that referenced this pull request May 24, 2022
scikit-image#6209) (scikit-image#6354)

* histogram_matching: use np.bincount for uints

`np.bincount` is preferable to `np.unique` for unsigned integer types (i.e. most image representations). This affords a large performance increase for these types of inputs.

* add benchmarks for match_histograms

* add float/int code paths consistency check

Co-authored-by: kwikwag <sadan.yuval@gmail.com>
lagru added a commit to lagru/scikit-image that referenced this pull request Oct 15, 2022
Add kwikwag due to his contributions in scikit-image#6209, which are included in
scikit-image#6354.
stefanv added a commit that referenced this pull request Jan 9, 2023
* Add automatically generated v0.20 release notes

produced by generate_release_notes.py. Still requires manual cleaning.

* Turn pr numbers into URLs

* Use template structure from release_dev.rst

* Fix urls

* Describe first PRs on footprint decomposition

* Remove PRs that were backported to v0.19

* Separate "New features" and "Changes"

"New features" MUST be totally backwards compatible. Everything else
goes into "changes and new deprecations".

* Apply suggestions from code review

Some suggestions I plan to edit locally, some I'd like to discuss first.

Co-authored-by: Stefan van der Walt <sjvdwalt@gmail.com>
Co-authored-by: Marianne Corvellec <marianne.corvellec@ens-lyon.org>

* Clarify some new features and address review

Use present tense and imperative tone for release notes. Try to be
more precise about why and how things changed.

Co-authored-by: Marianne Corvellec <marianne.corvellec@ens-lyon.org>
Co-authored-by: Stefan van der Walt <sjvdwalt@gmail.com>

* Add work on release notes

A lot of the removed PRs were already backported to 0.19. So I'm
ignoring these for 0.20. Note that I'm checking if the commit is
available in these releases with "git tag --contains <COMMIT_HASH>".

* Continue work on release notes

* Continue work on release notes

I'm confused with what to do abou the Milestone 1.0. Should these be
included in 0.20 now. Collect them for now until that's confirmed.

Furthermore, collect the backported PRs. We can delete them later but
for now it's useful to keep them for review and in case we want to go
back. Note that quite a lot are missing because I didn't start this
earlier.

* Continue work on release notes

* Fix too short title underline

* Continue work on 0.20 release notes

* Continue work on 0.20 release notes

* Continue work on 0.20 release notes

Add kwikwag due to his contributions in #6209, which are included in
#6354.

* Continue work on 0.20 release notes

* Continue work on 0.20 release notes

* Reword description of new footprint decomposition

Co-authored-by: Gregory Lee <grlee77@gmail.com>

* Continue work on 0.20 release notes

* Continue work on 0.20 release notes

* Continue work on 0.20 release notes

* Remove list of all PRs

* Add not included PRs as of 2022-10-24

Should include some unlisted backported PRs as well.

* Continue work on 0.20 release notes

* Finish including all current PRs

in 0.20's release notes.

* Reorder and -structure release notes

This introduces additional subsections as a suggestion. Hopefully this
makes it easier for readers to focus on the stuff they are actually
interested in without having to read through a list of minor fixes and
updates.

Were it made sense I also consolidated related bullet points. E.g. if
one PR added something new and a following one added some further fixes
or improvements. In that case both PR numbers are appended in brackets.

* Resolve mislabeled PRs in milestone 0.21

As these are already merged and included they should go into 0.20. I
don't think any of these (formerly 1.0) are breaking.

* Update author and reviewer lists

* Add recent merged PRs to 0.20 release notes

* Apply some suggestions from @mkcor

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

* Apply @mkcor's suggestion

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

* Apply modified suggestions from @mkcor

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

* Add recent merged PRs to 0.20 release notes

as of 2022-12-05.

* Continue work on release notes

* Fix contributor list

* Continue work on release notes

Only PR left is the one with the long list of completed deprecations.

* Apply suggestions from @mkcor

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

* Remove unnecessary "the" in a few places

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

* Continue work on release notes

* Add recently merged PR 6581

* Add recently merged PR 6644

* Add recently merged PR 6650

* Don't use ambiguous "dep"

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

* Add recently merged PR 6652

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

* Add recently merged PR 6655

* Add draft of the highlights section

See https://github.com/scikit-image/scikit-image/releases/tag/v0.19.0
for an example from an earlier release.

* Mark as prerelease

Co-authored-by: Stefan van der Walt <sjvdwalt@gmail.com>
Co-authored-by: Marianne Corvellec <marianne.corvellec@ens-lyon.org>
Co-authored-by: Gregory Lee <grlee77@gmail.com>
Co-authored-by: Jarrod Millman <jarrod.millman@gmail.com>
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