Skip to content

Commit

Permalink
Fix grouped covariance to require both values to be convertible to do…
Browse files Browse the repository at this point in the history
…uble. (#10891)

This PR changes a requirement to ensure that both value inputs to a sort-groupby covariance computation are convertible to double.

Authors:
  - Bradley Dice (https://github.com/bdice)

Approvers:
  - Nghia Truong (https://github.com/ttnghia)
  - https://github.com/nvdbaranec
  - David Wendt (https://github.com/davidwendt)

URL: #10891
  • Loading branch information
bdice authored May 28, 2022
1 parent 6e65fa1 commit 6025f2b
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 4 deletions.
5 changes: 2 additions & 3 deletions cpp/src/groupby/sort/group_correlation.cu
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ std::unique_ptr<column> group_covariance(column_view const& values_0,
.type();
};
bool const is_convertible =
type_dispatcher(get_base_type(values_0), is_double_convertible_impl{}) or
type_dispatcher(get_base_type(values_0), is_double_convertible_impl{}) and
type_dispatcher(get_base_type(values_1), is_double_convertible_impl{});

CUDF_EXPECTS(is_convertible,
Expand Down Expand Up @@ -185,8 +185,7 @@ std::unique_ptr<column> group_correlation(column_view const& covariance,
rmm::mr::device_memory_resource* mr)
{
using result_type = id_to_type<type_id::FLOAT64>;
CUDF_EXPECTS(covariance.type().id() == type_id::FLOAT64,
"Covariance result as FLOAT64 is supported");
CUDF_EXPECTS(covariance.type().id() == type_id::FLOAT64, "Covariance result must be FLOAT64");
auto stddev0_ptr = stddev_0.begin<result_type>();
auto stddev1_ptr = stddev_1.begin<result_type>();
auto stddev_iter = thrust::make_zip_iterator(thrust::make_tuple(stddev0_ptr, stddev1_ptr));
Expand Down
18 changes: 17 additions & 1 deletion cpp/tests/groupby/covariance_tests.cpp
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2021, NVIDIA CORPORATION.
* Copyright (c) 2021-2022, NVIDIA CORPORATION.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -43,6 +43,22 @@ using supported_types = RemoveIf<ContainedIn<Types<bool>>, cudf::test::NumericTy
TYPED_TEST_SUITE(groupby_covariance_test, supported_types);
using K = int32_t;

TYPED_TEST(groupby_covariance_test, invalid_types)
{
using V = TypeParam;

auto keys = fixed_width_column_wrapper<K>{{1, 2, 2, 1}};
auto member_0 = fixed_width_column_wrapper<V>{{1, 1, 1, 2}};
// Covariance aggregations require all types are convertible to double, but
// duration_D cannot be converted to double.
auto member_1 = fixed_width_column_wrapper<cudf::duration_D, cudf::duration_D::rep>{{0, 0, 1, 1}};
auto vals = structs{{member_0, member_1}};

auto agg = cudf::make_covariance_aggregation<groupby_aggregation>();
EXPECT_THROW(test_single_agg(keys, vals, keys, vals, std::move(agg), force_use_sort_impl::YES),
cudf::logic_error);
}

TYPED_TEST(groupby_covariance_test, basic)
{
using V = TypeParam;
Expand Down

0 comments on commit 6025f2b

Please sign in to comment.