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

Fix intermediate dtypes for float16 inputs in cupyx.scipy.ndimage stats functions #3402

Merged
merged 2 commits into from
Jun 9, 2020

Conversation

takagi
Copy link
Member

@takagi takagi commented Jun 5, 2020

Follows #3259 (comment).

NumPy's ndarray.sum() and ndarray.mean() use different intermediate dtypes for summation and return a little different results for float16 inputs.

import numpy as np
a = (np.arange(100) / 2).astype(np.float16)
>>> print(a.sum() / a.size)
24.76
>>> print(a.mean())
24.75

Because of this, cupyx.scipy.ndimage stats functions introduced in #3259 return slightly different results for float16 inputs compared with their SciPy counterparts. This PR fixes the functions to follow SciPy's way to compute such values.

@emcastillo emcastillo self-assigned this Jun 8, 2020
@emcastillo emcastillo added the cat:bug Bugs label Jun 8, 2020
@emcastillo emcastillo added this to the v8.0.0b4 milestone Jun 8, 2020

if cupy.isscalar(index):
count, sum_c_sq = single_group(input[labels == index])
return sum_c_sq / cupy.asanyarray(count).astype(float)
Copy link
Member

Choose a reason for hiding this comment

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

The three cases looks too similar
Couldn't be grouped in something like

def calc_var_with_intermediate_float(input):
    vals_c = input - input.mean()
    return cupy.square(vals_c).sum() / cupy.asanyarray(count).astype(float)


if cupy.isscalar(index):
count, sum = single_group(input[labels == index])
return sum / cupy.asanyarray(count).astype(float)
Copy link
Member

Choose a reason for hiding this comment

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

Same as above 😇

Copy link
Member

@emcastillo emcastillo left a comment

Choose a reason for hiding this comment

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

Left some comments, but I don't know if it makes sense 😅

@takagi
Copy link
Member Author

takagi commented Jun 9, 2020

I fixed them as well as mean!

Copy link
Member

@emcastillo emcastillo left a comment

Choose a reason for hiding this comment

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

LGTM

@emcastillo
Copy link
Member

Sorry, please fix travis!

@emcastillo
Copy link
Member

Mmm

�[91mWarning, treated as error:�[39;49;00m
/home/travis/build/cupy/cupy/docs/source/reference/generated/cupy.fft.fft.rst:6:Error in "autofunction" directive:
1 argument(s) required, 0 supplied.

.. autofunction::

@takagi
Copy link
Member Author

takagi commented Jun 9, 2020

Perhaps because of Running Sphinx v3.1.0?

@cupy cupy deleted a comment from pfn-ci-bot Jun 9, 2020
@emcastillo
Copy link
Member

has it been updated recently?

@takagi
Copy link
Member Author

takagi commented Jun 9, 2020

Yesterday. https://pypi.org/project/Sphinx/#history

@takagi
Copy link
Member Author

takagi commented Jun 9, 2020

I will post a PR to fix .travis.yml to read docs/requirements.txt .

@takagi takagi force-pushed the fix-ndimage-stats-float16 branch from 08351d7 to 442aa77 Compare June 9, 2020 04:53
@emcastillo
Copy link
Member

Jenkins, test this please

@pfn-ci-bot
Copy link
Collaborator

Successfully created a job for commit 442aa77:

@emcastillo emcastillo added the st:test-and-merge (deprecated) Ready to merge after test pass. label Jun 9, 2020
@chainer-ci
Copy link
Member

Jenkins CI test (for commit 442aa77, target branch master) succeeded!

@mergify mergify bot merged commit bedd677 into cupy:master Jun 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cat:bug Bugs st:test-and-merge (deprecated) Ready to merge after test pass.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants