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

Integral_image: promote floating point inputs to double by default #5392

Merged
merged 4 commits into from
Jun 5, 2021

Conversation

grlee77
Copy link
Contributor

@grlee77 grlee77 commented May 13, 2021

Description

This PR was split off from #5372 as it is a change in existing behavior. To avoid surprises due to poor accuracy of floating point cumulative sums, I propose we default to double precision when computing integral images. See additional details in prior comment: #5372 (comment).

The user can override the default by manually specifying the dtype argument.

One question is whether we can consider this a bug fix or if we should introduce a deprecation cycle before making the switch. The new output should be strictly an improvement from an accuracy standpoint, but output dtype will be larger for float16, float32 or complex64 inputs.

Checklist

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.

@grlee77 grlee77 added the ⏩ type: Enhancement Improve existing features label May 13, 2021
References
----------
.. [1] F.C. Crow, "Summed-area tables for texture mapping,"
ACM SIGGRAPH Computer Graphics, vol. 18, 1984, pp. 207-212.

"""
if dtype is None and image.real.dtype.kind == 'f':
# default to at least double precision cumsum for accuracy
dtype = np.promote_types(image.dtype, float)
Copy link
Member

Choose a reason for hiding this comment

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

I think on some Windows builds, "float" will default to float32. Should we specify np.float64 here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is the case for int on 32-bit, but I thought float was usually double precision? Definitely not a problem to just use np.float64 to make it unambiguous, though

skimage/transform/integral.py Outdated Show resolved Hide resolved
skimage/transform/integral.py Outdated Show resolved Hide resolved
@mkcor
Copy link
Member

mkcor commented Jun 2, 2021

One question is whether we can consider this a bug fix or if we should introduce a deprecation cycle before making the switch.

I'm happy to consider it a bug fix, since the change fixes floating-point cumulative sums which used to be off. Does @rfezzani agree?

The new output should be strictly an improvement from an accuracy standpoint, but output dtype will be larger for [...] complex64 inputs.

Which (larger) output dtype do complex64 inputs yield? I guess complex128 where the complex number is represented by two 64-bit floats?

@grlee77
Copy link
Contributor Author

grlee77 commented Jun 4, 2021

Which (larger) output dtype do complex64 inputs yield? I guess complex128 where the complex number is represented by two 64-bit floats?

Yes, this is correct.

Co-authored-by: Marianne Corvellec <marianne.corvellec@ens-lyon.org>
@jni jni merged commit ba32eaf into scikit-image:main Jun 5, 2021
@rfezzani
Copy link
Member

rfezzani commented Jun 7, 2021

One question is whether we can consider this a bug fix or if we should introduce a deprecation cycle before making the switch.

I'm happy to consider it a bug fix, since the change fixes floating-point cumulative sums which used to be off. Does @rfezzani agree?

The new output should be strictly an improvement from an accuracy standpoint, but output dtype will be larger for [...] complex64 inputs.

Which (larger) output dtype do complex64 inputs yield? I guess complex128 where the complex number is represented by two 64-bit floats?

Sorry for not being available these last weeks 🙏...

👍 I agree @mkcor, and thank you @juan for merging this 😉 and thank you again @grlee77 🎉

@grlee77 grlee77 deleted the integral_image_dtype branch July 8, 2021 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⏩ type: Enhancement Improve existing features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants