-
-
Notifications
You must be signed in to change notification settings - Fork 808
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 different dtype input test in histogram #3618
Conversation
It's not CUB's requirement, but actually NumPy's: Lines 585 to 589 in 0af4d97
|
cupy/statistics/histogram.py
Outdated
else: | ||
x = x.astype(bin_type, copy=False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need this. It's usually cheaper to change the bins than the input array, as the latter is often much larger.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I will fix!
bins = testing.shaped_arange((10,), xp, dtype_a) | ||
|
||
if xp is numpy: | ||
return xp.histogram(x, bins)[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we create another test, say test_histogram_with_bins2
, to test xp.histogram(x, bins)[1]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added.
cupy/statistics/histogram.py
Outdated
bin_type = numpy.float | ||
else: | ||
x = x.astype(bin_type, copy=False) | ||
acc_bin_edge = bin_edges.astype(bin_type, copy=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do I understand correctly you wanna hide the cost of the second bin shift by creating a copy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
cupy/statistics/histogram.py
Outdated
assert isinstance(bin_edges, cupy.ndarray) | ||
bin_type = numpy.result_type(bin_edges.dtype, x.dtype) | ||
if numpy.issubdtype(x.dtype, numpy.integer): | ||
bin_type = numpy.float |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is cleaner 👍 But, when _get_bin_edges()
generates bin_edges
, if bins
is int
this type requirement is already satisfied, so it's why we first test for if isinstance(bins, cupy.ndarray):
. Would be nice to keep the if branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Certainly, it seems to be faster.
The point is that bins needs type conversion even when numpy.array
or list
is an input.
How about if not is instance(bins, int):
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah let's prepare for #3542.
pfnCI, test this please. |
Successfully created a job for commit 4ea497f: |
Jenkins CI test (for commit 4ea497f, target branch master) failed with status FAILURE. |
The CI also gets wrong with float32? >>> import cupy
>>> x = cupy.testing.shaped_arange((10,), cupy, 'f')
>>> bins = cupy.arange(1, 10.1, 0.9)
>>> bins[-1] = 10.1
>>> y = cupy.empty(10, 'l')
>>> cupy.cuda.cub.device_histogram(x, bins, y)
array([1, 1, 0, 0, 0, 0, 0, 8, 0, 0])
>>> bins = bins.astype('f')
>>> cupy.cuda.cub.device_histogram(x, bins, y)
array([1, 1, 1, 1, 1, 1, 1, 1, 1, 1]) |
We have to use double type |
In @takagi's snippet, |
Blocked by #3617 |
As #3617 is merged to the master now, would you resolve the conflict? |
4ea497f
to
229e52e
Compare
I fixed conflicts. So, this PR only adds testcase |
pfnCI, test this please. |
Jenkins CI test (for commit 229e52e, target branch master) failed with status FAILURE. |
pfnCI, test this please. |
Jenkins CI test (for commit 229e52e, target branch master) failed with status FAILURE. |
pfnCI, test this please. |
Jenkins CI test (for commit 229e52e, target branch master) failed with status FAILURE. |
Waiting for CI problem fixed. |
pfnCI, test this please. |
Jenkins CI test (for commit 229e52e, target branch master) failed with status FAILURE. |
Co-authored-by: Leo Fang <leofang@bnl.gov>
229e52e
to
306ed6f
Compare
pfnCI, test this please. |
Jenkins CI test (for commit 306ed6f, target branch master) succeeded! |
LGTM! |
Add different dtype input test in histogram
cub.device_histogram
assumes following dtype combination.x
andbin_edge
x
isinteger
dtype,binedge
isdouble
dtype