Skip to content

Let lower precision float arrays pass through img_as_float#3110

Merged
jni merged 2 commits intoscikit-image:masterfrom
stefanv:float_to_float_conversion
May 29, 2018
Merged

Let lower precision float arrays pass through img_as_float#3110
jni merged 2 commits intoscikit-image:masterfrom
stefanv:float_to_float_conversion

Conversation

@stefanv
Copy link
Copy Markdown
Member

@stefanv stefanv commented May 28, 2018

This is an alternative approach to https://github.com/scikit-image/scikit-image/pull/3062/files

It also led me to identify a precision issue in the conversion from signed int -> float.

stefanv added 2 commits May 28, 2018 15:01
Adding small floating point numbers can cause inaccuracies.  Reordered the
computation to do a multiplication instead.
@pep8speaks
Copy link
Copy Markdown

Hello @stefanv! Thanks for submitting the PR.

Line 139:1: E302 expected 2 blank lines, found 1
Line 140:24: E241 multiple spaces after ','
Line 144:1: E302 expected 2 blank lines, found 1

@codecov-io
Copy link
Copy Markdown

codecov-io commented May 28, 2018

Codecov Report

Merging #3110 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3110      +/-   ##
==========================================
+ Coverage   85.91%   85.93%   +0.01%     
==========================================
  Files         336      336              
  Lines       27316    27336      +20     
==========================================
+ Hits        23469    23490      +21     
+ Misses       3847     3846       -1
Impacted Files Coverage Δ
skimage/util/tests/test_dtype.py 100% <100%> (ø) ⬆️
skimage/util/dtype.py 89.75% <100%> (+0.73%) ⬆️
skimage/measure/tests/test_regionprops.py 100% <0%> (ø) ⬆️
skimage/measure/_regionprops.py 97.82% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be42f61...00adce2. Read the comment docs.

Copy link
Copy Markdown
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.

👍

@jni jni merged commit a805196 into scikit-image:master May 29, 2018
and can be outside the ranges [0.0, 1.0] or [-1.0, 1.0].

"""
return convert(image, np.floating, force_copy)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Interesting, good call on the subclass!

image = np.multiply(image, 2. / (imax_in - imin_in),
dtype=computation_type)
image += 1.0 / (imax_in - imin_in)
image = np.add(image, 0.5, dtype=computation_type)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Did this really lead to an inacurracy?

Interesting.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, it did. See the np.int8 test case that @stefanv added. It fails without this change, due to floating point arithmetic error.

@hmaarrfk
Copy link
Copy Markdown
Member

@jni is there a chance this can be backported to 0.14.1?

@jni
Copy link
Copy Markdown
Member

jni commented Jun 13, 2018

@meeseeksdev backport to v0.14.x

@jni
Copy link
Copy Markdown
Member

jni commented Jun 13, 2018

@hmaarrfk apparently it can. =D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants