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 simple test for center_of_mass #122

Merged
merged 6 commits into from
Oct 14, 2019

Conversation

Mause
Copy link
Contributor

@Mause Mause commented Aug 5, 2019

Before you submit a pull request, check that it meets these guidelines:

  1. The pull request should include tests.
  2. If the pull request adds functionality, the docs should be updated. Put
    your new functionality into a function with a docstring, and add the
    feature to the list in README.rst.
  3. The pull request should work for Python 3.5, 3.6, and 3.7. Check
    https://travis-ci.org/dask/dask-image/pull_requests
    and make sure that the tests pass for all supported Python versions.

@GenevieveBuckley
Copy link
Collaborator

GenevieveBuckley commented Aug 5, 2019

Ah, I see why you were not sure if the issue contained a legitimate bug.

So, we only see the bug if an array containing integer values is passed to dask_image.ndmeasure.center_of_mass(). The array you make in this test has float values in it, since numpy.random.random() outputs arrays of float values.

For this test to show the bug, it would need to go something like this:

a = np.random.random(shape) * 255 # multiply so values are in 8-bit range 0-255
a = a.astype(np.uint8)  # convert to unsigned 8-bit integer data
d = da.from_array(a, chunks=chunks)

This also means that the actual fix for the issue should also be a single line, converting the array chunks to float type before running the computation in `dask_image.ndmeasure.center_of_mass()

@GenevieveBuckley
Copy link
Collaborator

I think the fix should happen in this line of dask-image/dask_image/ndmeasure/__init__.py:

out_dtype = numpy.dtype([("com", image.dtype, (image.ndim,))])

image.dtype needs to be changed to always be float type (instead of just reflecting the type of the input array like it does now). That's probably going to look like this instead:

out_dtype = numpy.dtype([("com", float, (image.ndim,))])

@Mause
Copy link
Contributor Author

Mause commented Aug 6, 2019

I figured the issue was the data I was testing with, but wasn't sure how to fix it - I'll do that now

@GenevieveBuckley GenevieveBuckley merged commit e77c0f6 into dask:master Oct 14, 2019
@GenevieveBuckley
Copy link
Collaborator

Thanks @Mause!

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.

2 participants