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.

Sign up for GitHub

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 incorrect results from cumulative_sum and cumulative_prod with dtype=bool #2018

Merged
merged 6 commits into from
Feb 28, 2025
+45 −11
Merged

Fix incorrect results from cumulative_sum and cumulative_prod with dtype=bool #2018

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e2d2b3f
Add missing kernels to arithmetic accumulators
ndgrigorian Feb 25, 2025
47239cd
Add tests based on gh-2017
ndgrigorian Feb 25, 2025
48b0e66
Add comment specifying need for work-around in accumulators
ndgrigorian Feb 26, 2025
c58d775
Reuse SYCL utils in workaround for logical operators in accumulators
ndgrigorian Feb 26, 2025
35d46d3
Alias conditional binary op type for cumulative_sum and cumulative_prod
ndgrigorian Feb 26, 2025
fa525a7
Add comment to needs_workaround in accumulators.hpp
ndgrigorian Feb 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion dpctl/tensor/libtensor/include/kernels/accumulators.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,9 +84,18 @@ template <typename srcTy, typename dstTy> struct CastTransformer
}
};

template <typename ScanOpT, typename T> struct needs_workaround
{
// work-around needed due to crash in JITing on CPU
static constexpr bool value =
std::is_same_v<ScanOpT, sycl::logical_or<T>> ||
std::is_same_v<ScanOpT, sycl::logical_and<T>>;
};

template <typename BinOpT, typename T> struct can_use_inclusive_scan_over_group
{
static constexpr bool value = sycl::has_known_identity<BinOpT, T>::value;
static constexpr bool value = sycl::has_known_identity<BinOpT, T>::value &&
!needs_workaround<BinOpT, T>::value;
};

namespace detail
Expand Down
18 changes: 14 additions & 4 deletions dpctl/tensor/libtensor/source/accumulators/cumulative_prod.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,10 +70,12 @@ template <typename argTy, typename outTy>
struct TypePairSupportDataForProdAccumulation
{
static constexpr bool is_defined = std::disjunction<
td_ns::TypePairDefinedEntry<argTy, bool, outTy, bool>,
td_ns::TypePairDefinedEntry<argTy, bool, outTy, std::int32_t>,
td_ns::TypePairDefinedEntry<argTy, bool, outTy, std::int64_t>,

// input int8_t
td_ns::TypePairDefinedEntry<argTy, std::int8_t, outTy, std::int8_t>,
td_ns::TypePairDefinedEntry<argTy, std::int8_t, outTy, std::int32_t>,
td_ns::TypePairDefinedEntry<argTy, std::int8_t, outTy, std::int64_t>,

Expand Down Expand Up @@ -138,7 +140,9 @@ struct CumProd1DContigFactory
if constexpr (TypePairSupportDataForProdAccumulation<srcTy,
dstTy>::is_defined)
{
using ScanOpT = sycl::multiplies<dstTy>;
using ScanOpT = std::conditional_t<std::is_same_v<dstTy, bool>,
sycl::logical_and<dstTy>,
sycl::multiplies<dstTy>>;
constexpr bool include_initial = false;
if constexpr (std::is_same_v<srcTy, dstTy>) {
using dpctl::tensor::kernels::accumulators::NoOpTransformer;
Expand Down Expand Up @@ -171,7 +175,9 @@ struct CumProd1DIncludeInitialContigFactory
if constexpr (TypePairSupportDataForProdAccumulation<srcTy,
dstTy>::is_defined)
{
using ScanOpT = sycl::multiplies<dstTy>;
using ScanOpT = std::conditional_t<std::is_same_v<dstTy, bool>,
sycl::logical_and<dstTy>,
sycl::multiplies<dstTy>>;
constexpr bool include_initial = true;
if constexpr (std::is_same_v<srcTy, dstTy>) {
using dpctl::tensor::kernels::accumulators::NoOpTransformer;
Expand Down Expand Up @@ -204,7 +210,9 @@ struct CumProdStridedFactory
if constexpr (TypePairSupportDataForProdAccumulation<srcTy,
dstTy>::is_defined)
{
using ScanOpT = sycl::multiplies<dstTy>;
using ScanOpT = std::conditional_t<std::is_same_v<dstTy, bool>,
sycl::logical_and<dstTy>,
sycl::multiplies<dstTy>>;
constexpr bool include_initial = false;
if constexpr (std::is_same_v<srcTy, dstTy>) {
using dpctl::tensor::kernels::accumulators::NoOpTransformer;
Expand Down Expand Up @@ -237,7 +245,9 @@ struct CumProdIncludeInitialStridedFactory
if constexpr (TypePairSupportDataForProdAccumulation<srcTy,
dstTy>::is_defined)
{
using ScanOpT = sycl::multiplies<dstTy>;
using ScanOpT = std::conditional_t<std::is_same_v<dstTy, bool>,
sycl::logical_and<dstTy>,
sycl::multiplies<dstTy>>;
constexpr bool include_initial = true;
if constexpr (std::is_same_v<srcTy, dstTy>) {
using dpctl::tensor::kernels::accumulators::NoOpTransformer;
Expand Down
18 changes: 14 additions & 4 deletions dpctl/tensor/libtensor/source/accumulators/cumulative_sum.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,10 +70,12 @@ template <typename argTy, typename outTy>
struct TypePairSupportDataForSumAccumulation
{
static constexpr bool is_defined = std::disjunction<
td_ns::TypePairDefinedEntry<argTy, bool, outTy, bool>,
td_ns::TypePairDefinedEntry<argTy, bool, outTy, std::int32_t>,
td_ns::TypePairDefinedEntry<argTy, bool, outTy, std::int64_t>,

// input int8_t
td_ns::TypePairDefinedEntry<argTy, std::int8_t, outTy, std::int8_t>,
td_ns::TypePairDefinedEntry<argTy, std::int8_t, outTy, std::int32_t>,
td_ns::TypePairDefinedEntry<argTy, std::int8_t, outTy, std::int64_t>,

Expand Down Expand Up @@ -138,7 +140,9 @@ struct CumSum1DContigFactory
if constexpr (TypePairSupportDataForSumAccumulation<srcTy,
dstTy>::is_defined)
{
using ScanOpT = sycl::plus<dstTy>;
using ScanOpT =
std::conditional_t<std::is_same_v<dstTy, bool>,
sycl::logical_or<dstTy>, sycl::plus<dstTy>>;
constexpr bool include_initial = false;
if constexpr (std::is_same_v<srcTy, dstTy>) {
using dpctl::tensor::kernels::accumulators::NoOpTransformer;
Expand Down Expand Up @@ -171,7 +175,9 @@ struct CumSum1DIncludeInitialContigFactory
if constexpr (TypePairSupportDataForSumAccumulation<srcTy,
dstTy>::is_defined)
{
using ScanOpT = sycl::plus<dstTy>;
using ScanOpT =
std::conditional_t<std::is_same_v<dstTy, bool>,
sycl::logical_or<dstTy>, sycl::plus<dstTy>>;
constexpr bool include_initial = true;
if constexpr (std::is_same_v<srcTy, dstTy>) {
using dpctl::tensor::kernels::accumulators::NoOpTransformer;
Expand Down Expand Up @@ -204,7 +210,9 @@ struct CumSumStridedFactory
if constexpr (TypePairSupportDataForSumAccumulation<srcTy,
dstTy>::is_defined)
{
using ScanOpT = sycl::plus<dstTy>;
using ScanOpT =
std::conditional_t<std::is_same_v<dstTy, bool>,
sycl::logical_or<dstTy>, sycl::plus<dstTy>>;
constexpr bool include_initial = false;
if constexpr (std::is_same_v<srcTy, dstTy>) {
using dpctl::tensor::kernels::accumulators::NoOpTransformer;
Expand Down Expand Up @@ -237,7 +245,9 @@ struct CumSumIncludeInitialStridedFactory
if constexpr (TypePairSupportDataForSumAccumulation<srcTy,
dstTy>::is_defined)
{
using ScanOpT = sycl::plus<dstTy>;
using ScanOpT =
std::conditional_t<std::is_same_v<dstTy, bool>,
sycl::logical_or<dstTy>, sycl::plus<dstTy>>;
constexpr bool include_initial = true;
if constexpr (std::is_same_v<srcTy, dstTy>) {
using dpctl::tensor::kernels::accumulators::NoOpTransformer;
Expand Down
12 changes: 12 additions & 0 deletions dpctl/tests/test_tensor_accumulation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -421,3 +421,15 @@ def test_cumulative_sum_gh_1901(p):
inp = dpt.ones(n, dtype=dt)
r = dpt.cumulative_sum(inp, dtype=dt)
assert dpt.all(r == dpt.arange(1, n + 1, dtype=dt))


@pytest.mark.parametrize(
"dt", ["i1", "i2", "i4", "i8", "f2", "f4", "f8", "c8", "c16"]
)
def test_gh_2017(dt):
"See https://github.com/IntelPython/dpctl/issues/2017"
q = get_queue_or_skip()
skip_if_dtype_not_supported(dt, q)
x = dpt.asarray([-1, 1], dtype=dpt.dtype(dt), sycl_queue=q)
r = dpt.cumulative_sum(x, dtype="?")
assert dpt.all(r)
Loading