Fix DeviceHistogram::Even for mixed float/int levels and sample types. #487
Conversation
alliepiper
added
type: bug: functional
|
gpuCI: NVIDIA/thrust#1697 |
|
@Melirius This should fix your issue with |
alliepiper
added
testing: gpuCI passed
|
It is nice, thanks for your work! However, it does not address another problem #489 that is rooted in the same code. |
There was a problem hiding this comment.
Thank you for starting the work on this! It seems that no one looked at the code for a while 😄
Regarding the PR, it seems to introduce some substantial changes. I'd like us to consider all implications before merging this PR. For instance, documentation doesn't say much about SampleT and LevelT. Unlike ConterT, LevelT isn't even required to be primitive. Let's gather requirements on these types, update documentation / assert these requirements and then have another review.
alliepiper
added
the
release: breaking change
alliepiper
force-pushed
the
histogram_mixed_type/gh.479
branch
from
b0b8adb to
ff1b254
Compare
alliepiper
added
testing: gpuCI in progress
alliepiper
force-pushed
the
histogram_mixed_type/gh.479
branch
from
ff1b254 to
c765c89
Compare
| #include <cub/util_cpp_dialect.cuh> | ||
|
|
||
| #if CUB_CPP_DIALECT >= 2017 && __cpp_if_constexpr | ||
| # define CUB_IF_CONSTEXPR if constexpr |
There was a problem hiding this comment.
Just to be sure, we cannot pass -Wno-c++17-extensions to our build flags
Asking for a friend
There was a problem hiding this comment.
No -- Thrust/CUB are header only, so that would require our users to do the same.
alliepiper
force-pushed
the
histogram_mixed_type/gh.479
branch
from
c765c89 to
16288ce
Compare
alliepiper
force-pushed
the
histogram_mixed_type/gh.479
branch
from
16288ce to
cf20c71
Compare
alliepiper
force-pushed
the
histogram_mixed_type/gh.479
branch
from
cf20c71 to
24b675d
Compare
alliepiper
force-pushed
the
histogram_mixed_type/gh.479
branch
from
24b675d to
3b3bf92
Compare
The
ScaleTransformutility precomputes the reciprocal of the "sample->bin_idx" scaling factor for floating point types as an optimization.Trouble is, the
Initmethod checked whetherLevelTis fp whileBinSelectcheckedSampleT. This caused the optimization to be incorrectly applied when one of these types is fp but the other is not.Fixed this bug and simplified the implementation of
ScaleTransformusingcuda::std::common_typeto consistently apply the optimization when eitherLevelTorScaleTare fp.Fixes #479 and #489 and adds regression tests for both.
Breaking Changes:
DeviceHistogram::HistogramEvenalgorithms when the samples and levels are different types (cub::DeviceHistogram::HistogramEven: Incorrect result whenLevelTdoes not exactly matchSampleT#479) and when both are integral with fractional bin sizes (cub::DeviceHistogram::HistogramEven: Incorrect result with integer levels and samples #489). These fixes introduced the following explicit restrictions:SampleTandLevelTmust support common arithmetic operations.cuda::std::common_type<SampleT, LevelT>must have a valid definition.int.