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

Add float32 support to moments_hu #5200

Merged
merged 1 commit into from Mar 23, 2021

Conversation

rfezzani
Copy link
Member

Description

Related to #5199.
Fused types are used to allow processing single precision values.

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.

@rfezzani rfezzani added the ⏩ type: Enhancement Improve existing features label Jan 28, 2021
@rfezzani
Copy link
Member Author

I wonder if maintaining the cython implementation of moments_hu worth it 😕.

I made some tests by implementing a pure python equivalent function:

def py_moments_hu(x):
    offset = x.shape[1]
    _x = x.reshape(-1)

    x03, x12, x21, x30 = _x[3: 3 * offset + 1: offset - 1]
    t0 = x30 + x12
    t1 = x21 + x03
    q0 = t0 * t0
    q1 = t1 * t1

    x02, x11, x20 = _x[2: 2 * offset + 1: offset - 1]
    n4 = 4 * x11
    s = x20 + x02
    d = x20 - x02

    hu = np.empty((7, ), x.dtype)
    hu[0] = s
    hu[1] = d * d + n4 * x11

    hu[3] = q0 + q1
    hu[5] = d * (q0 - q1) + n4 * t0 * t1

    t0 *= q0 - 3 * q1
    t1 *= 3 * q0 - q1
    q0 = x30 - 3 * x12
    q1 = 3 * x21 - x03

    hu[2] = q0 * q0 + q1 * q1
    hu[4] = q0 * t0 + q1 * t1
    hu[6] = q1 * t0 - q0 * t1

    return hu

I then performed some timing:

In [2]: x = np.random.rand(4, 4)                                                                                   

In [3]: %timeit py_moments_hu(x)                                                                    
7.3 µs ± 37.6 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)

In [4]: from skimage.measure import moments_hu                                                           

In [5]: %timeit moments_hu(x)                                                                            
4.33 µs ± 20.6 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)

Pure python version is as expected slower then cython implementation (1.69 times), but we are talking about µseconds here...

@rfezzani
Copy link
Member Author

This PR replicate #4307 (I forgot about it 😅).

@grlee77
Copy link
Contributor

grlee77 commented Jan 28, 2021

Thanks @rfezzani, is there any benefit to this PR over the prior one? (i.e. which would you like to have reviewed?).

I am also interested in improving preservation of float32 in general. In working on cupyimg I found quite a few places on the python side where scikit-image converts to float64 when not strictly necessary. I didn't see a currently open issue on this, so I will open one where we can list functions that should have single precision support and keep track of open PRs needing review.

@rfezzani
Copy link
Member Author

is there any benefit to this PR over the prior one?

As you already noticed, #4307 use the dtype parameter but this one not. Choose the option you prefer, I will close the other 😉

Copy link
Contributor

@grlee77 grlee77 left a comment

Choose a reason for hiding this comment

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

This looks good to me. I prefer this approach to the one in #4307 (I think it is cleaner without adding a new dtype argument to the API)

skimage/measure/_moments_cy.pyx Show resolved Hide resolved
@@ -345,7 +345,8 @@ def moments_hu(nu):
array([7.45370370e-01, 3.51165981e-01, 1.04049179e-01, 4.06442107e-02,
2.64312299e-03, 2.40854582e-02, 4.33680869e-19])
"""
return _moments_cy.moments_hu(nu.astype(np.double))
dtype = np.float32 if nu.dtype == 'float32' else np.float64
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems fine, but I wonder if we should explicitly raise on error on complex-valued inputs? I think there are probably a ton of other places where this is just assumed and not explicitly checked, thgouh. not sure it is worth it

if nu.dtype.kind = 'c':
    raise TValueError("complex-valued inputs are not suppoted.")

Copy link
Member Author

Choose a reason for hiding this comment

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

I have no strict opinion on this, but most of skimage functions do not explicitly check for complex input values, I am not sure it worth it 😕

@rfezzani rfezzani mentioned this pull request Feb 8, 2021
9 tasks
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.

I'm ok with the PR as is. Thanks @rfezzani!

Yeah maybe we should use pure Python but this is working and is faster so 🤷

@grlee77 grlee77 added this to In progress in skimage2 API Feb 15, 2021
Base automatically changed from master to main February 18, 2021 18:23
@grlee77 grlee77 merged commit abef5ce into scikit-image:main Mar 23, 2021
@grlee77 grlee77 moved this from In progress to Done in skimage2 API Mar 23, 2021
@rfezzani
Copy link
Member Author

Thank you @grlee77 🎉

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
Development

Successfully merging this pull request may close these issues.

None yet

3 participants