Skip to content

add int64 support to filters.rank_order and morphology.reconstruction#6342

Merged
alexdesiqueira merged 11 commits intoscikit-image:mainfrom
grlee77:grayreconstruct-int64
Jun 14, 2022
Merged

add int64 support to filters.rank_order and morphology.reconstruction#6342
alexdesiqueira merged 11 commits intoscikit-image:mainfrom
grlee77:grayreconstruct-int64

Conversation

@grlee77
Copy link
Copy Markdown
Contributor

@grlee77 grlee77 commented Apr 14, 2022

Description

closes #6277

This PR resumes work from #6295, using fused types to support both 32 and 64-bit integer types in morphology.reconstruction and filters.rank_order. This allows extremely large images to be used with these functions.

Additional changes:

  • skimage.filters.rank_order now uses the minimum required unsigned integer size rather than always using uint32
  • to minimize increase in binary size, I only implemented int32 and int64 for the Cython code.
  • single precision seed now returns single precision output (this function was missed during previous float32 work)
  • benchmarks were added

Performance remains approximately the same as before. Memory usage is now a bit less for float32 inputs.

Pythran version

In the earlier commits, I tried converting to Pythran, but measured 5-10x slower performance in that case for the larger sizes in the benchmarks. @serge-sans-paille, if you are interested in taking a look see commit d5295bc which has both Cython and pythran versions present (which one is called can be chosen by editing the import at the top of grayreconstruct.py). The pythran code is a bit cleaner (no cdefs required), but it is not obvious to me where the performance difference comes from.

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 grlee77 added the 🩹 type: Bug fix Fixes unexpected or incorrect behavior label Apr 14, 2022
@pep8speaks
Copy link
Copy Markdown

pep8speaks commented Apr 14, 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-06-09 15:39:56 UTC

@serge-sans-paille
Copy link
Copy Markdown
Contributor

@grlee77 I think the issue is that pythran cannot prove that array accesses are inbound. I'll investigate that :-)

@JamesSample
Copy link
Copy Markdown
Contributor

This looks fantastic @grlee77 !

Thanks for your help with this! Especially for the stuff involving fused types in Cython - it's great to have a version that chooses the most appropriate type automatically.

@grlee77 grlee77 added this to the 1.0 milestone Apr 19, 2022
Copy link
Copy Markdown
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, thank you @grlee77 😉

@alexdesiqueira
Copy link
Copy Markdown
Member

Thank you @grlee77 @JamesSample!

@lagru lagru modified the milestones: 0.21, 0.20 Oct 15, 2022
@lagru lagru mentioned this pull request Oct 15, 2022
4 tasks
jmuhlich added a commit to jmuhlich/scikit-image that referenced this pull request Nov 10, 2025
While scikit-image#6342 added code to support passing int64 arrays to
reconstruction_loop, the local variables that hold values from these
arrays are still 32-bit. This change uses the appropriate fused int
types instead.
stefanv pushed a commit that referenced this pull request Nov 10, 2025
I was experiencing segfaults in the reconstruction_loop cython code when
reconstruction was called with very large images, even after #6342 added
code to support passing int64 arrays. I discovered that the local
variables that hold the values from these arrays are still 32-bit. This
change uses the appropriate fused int types instead.

---------

Co-authored-by: Lars Grüter <lagru@mailbox.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🩹 type: Bug fix Fixes unexpected or incorrect behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Maximum array size for morphology.reconstruction?

7 participants