[REVIEW] Added GDF_BOOL type to cuda code #817

BradReesWork · 2019-01-30T22:01:18Z

Added a GDF_BOOL type.

The type was added as a wrapper type that resolves to an int8

update types.h
update wrapper_types.hpp
update type_dispatcher.hpp
update tests/type_test.cpp

This PR does not change any code to start using the new type

closes #667

cpp/src/utilities/wrapper_types.hpp

eyalroz

Things I'm missing:

All cudf code which uses GDF_INT8 but really uses it as boolean should be converted to use GDF_BOOL.
Doxygen comments regarding functions taking or producing columns of booleans (and I don't mean valid pseudo-columns, since they're bit-packed) should be given a once-over to check if they say the input or output has type GDF_BOOL.

Bottom line: I would "request changes", but I don't have the official capacity for that.

eyalroz · 2019-01-31T00:09:25Z

cpp/include/cudf/types.h

@@ -27,6 +27,7 @@ typedef enum {
    GDF_TIMESTAMP,	/**< Exact timestamp encoded with int64 since UNIX epoch (Default unit millisecond) */
    GDF_CATEGORY,
    GDF_STRING,
+    GDF_BOOL,


Wait, where's the typedef int8_t gdf_bool? Or rather, for consistency with the (problematic) typedefs we have now, typedef bool gdf_bool?

Also - Please add a comment explaining the semantics of this type, i.e. the fact that 0 means false and anything else means true.

I am not sure why we would want all GDF_INT8 code to be referenced as GDF_BOOL? While the datatype might be the same, conceptually they are different.

"the fact that 0 means false and anything else means true." has always been a C standard

I am not sure why we would want all GDF_INT8 code to be referenced as GDF_BOOL? While the datatype might be the same, conceptually they are different.

That's not what @eyalroz was suggesting. Instead he was just saying to add a typedef int8_t gdf_bool and then use that typedef in the wrapper as

using cudf_bool = detail::wrapper<gdf_bool, GDF_BOOL>;

"the fact that 0 means false and anything else means true." has always been a C standard

While this is true, all of the operators for wrapper don't behave this way. Thus, there's a few options:

Add specializations for the pertinent operator::wrapper for gdf_bool such that 0 means false and not zero means true (this is the most work)

Require that for gdf_bool that 0 means false and 1 means true, and all other values are invalid. (Could be done via assert or just trusting the user?)

Use typedef bool gdf_bool which automatically enforces 2., but is problematic because of the machine dependence on sizeof(bool).

Add a normalize function similar in concept to unwrap in that for all types other than gdf_bool, it's just a no-op, but for gdf_bool types, it checks the value of the gdf_bool, if 0 it stays 0, if not zero, it changes it to 1. You would then need to update all of the necessary wrappers operators to then call normalize on the passed in arguments.

I'll say I don't like the idea of keeping having cudf_foo and gdf_foo. The latter identifiers should all eventually become cudf_foo, and we'll be stuck with a conflict.

Also - @jrhemstad , in option (1.) you mean implementing arithmetic operators, comparison operators etc? I don't think that is really too much work.

I'm in favor of (1.) , but possibly also of (4.)

Also - @jrhemstad , in option (1.) you mean implementing arithmetic operators, comparison operators etc? I don't think that is really too much work.

Yes, it's not a lot of work, just more than 2, 3, or 4.

eyalroz · 2019-01-31T00:10:29Z

cpp/src/utilities/type_dispatcher.hpp

@@ -107,6 +107,7 @@ decltype(auto) type_dispatcher(gdf_dtype dtype,
    case GDF_DATE64:    { return f.template operator()< date64 >(std::forward<Ts>(args)...); }
    case GDF_TIMESTAMP: { return f.template operator()< timestamp >(std::forward<Ts>(args)...); }
    case GDF_CATEGORY:  { return f.template operator()< category >(std::forward<Ts>(args)...); }
+    case GDF_BOOL:      { return f.template operator()< gdf_bool >(std::forward<Ts>(args)...); }


Note to @jrhemstad : Another reason to put some priority on accepting fs which only have operator() implemented for a subset of the types. Hint hint.

cpp/src/utilities/wrapper_types.hpp

eyalroz · 2019-01-31T00:19:11Z

cpp/src/utilities/wrapper_types.hpp

@@ -263,6 +263,9 @@ using date32 = detail::wrapper<gdf_date32, GDF_DATE32>;

 using date64 = detail::wrapper<gdf_date64, GDF_DATE64>;

+using gdf_bool 	= detail::wrapper<int8_t, GDF_BOOL>;


To be honest, I don't really like how some cudf types have using statements here, and some don't. But while that's the rule - why should gdf_bool be different than the integer and floating-point types? It does correspond to a C/C++ native type after all.

I believe it would be problematic to use bool natively, because the size of bool in C++ is implementation defined.

Of course it would! ... I was just being consistent with the other problematic typedef's:

typedef int gdf_size_type; /**< Limits the maximum size of a gdf_column to 2^31-1 */ typedef gdf_size_type gdf_index_type; typedef unsigned char gdf_valid_type; typedef long gdf_date64; typedef int gdf_date32; typedef int gdf_category; typedef long gdf_timestamp;

int and long are also of implementation-defined size... I'd be all for

typedef int32_t gdf_size_type; typedef gdf_size_type gdf_index_type; typedef uint8_t gdf_valid_type; /* shouldn't this become uint32_t ? */ typedef int64_t gdf_date64; typedef int32_t gdf_date32; typedef int64_t gdf_timestamp; typedef int32_t gdf_category; typedef int8_t gdf_bool;

The initial conversation was around rather to simply do a typedef int8_t gdf_bool or to use the type_dispatcher. We had decided to go down the type_dispatcher route, hence the is no typedef.

@BradReesWork : I don't quite understand. How does it make sense to have typedef int gdf_date32; but not have typedef signed char gdf_bool or typedef int8_t gdf_bool?

eyalroz · 2019-01-31T00:20:08Z

cpp/tests/types/types_test.cpp

@@ -49,7 +49,7 @@ struct WrappersTest : public ::testing::Test {
 };

 using Wrappers = ::testing::Types<cudf::category, cudf::timestamp, cudf::date32,
-                                  cudf::date64>;
+                                  cudf::date64, cudf::gdf_bool>;


That's it, no other tests? I wonder if this is enough.

I agree that more test would be beneficial. The current set of test does run through the most of the standard mathematical operations. What is missing are boolean operators. I will get those added

jrhemstad · 2019-01-31T00:37:17Z

@jrhemstad: What implicit integral upcasting for types smaller than int?

@eyalroz referring to this: https://en.cppreference.com/w/cpp/language/implicit_conversion#Integral_promotion

Specifically:

In particular, arithmetic operators do not accept types smaller than int as arguments, and integral promotions are automatically applied

This was problematic because of the following:

template <typename T>
wrapper<T> operator+(wrapper<T> const &lhs, wrapper<T> const &rhs)
{
     return wrapper<T>{lhs.value + rhs.value};
}

This would throw a warning (error, since all warnings are errors) when T == int8_t because of a narrowing conversion from int to int8_t.

Basically, this will fail for wrapper<T,dtype> when T is smaller than an int.

static_assert(std::is_same<T, decltype(lhs.value + rhs.value)>::value);

Example here: https://wandbox.org/permlink/pU48n53H8F76g7SP

jrhemstad

I just realized that operator== will need a specialization for cudf::gdf_bool, because it should be such that if both lhs and rhs are greater than zero, then they are equal.

There will need to be some changes to the unit tests to test this as well.

kkraus14 · 2019-01-31T19:39:02Z

I just realized that operator== will need a specialization for cudf::gdf_bool, because it should be such that if both lhs and rhs are greater than zero, then they are equal.

There will need to be some changes to the unit tests to test this as well.

In theory shouldn't it be incorrect behavior to have any value other than a 1 or 0 in a GDF_BOOL column? I understand if comparing a bool to an int8 for example, but wouldn't that be a different function / operator?

eyalroz · 2019-02-01T00:00:04Z

I just realized that operator== will need a specialization for cudf::gdf_bool, because it should be such that if both lhs and rhs are greater than zero, then they are equal.

So, also all other comparison operators need to be changed as well.

Also, we might want to consider a normalize() or canonicalize() method, which for GDF_BOOL would replace non-zero values with 1's and keep 0's as-is.

jrhemstad · 2019-02-01T18:25:04Z

I just realized that operator== will need a specialization for cudf::gdf_bool, because it should be such that if both lhs and rhs are greater than zero, then they are equal.

So, also all other comparison operators need to be changed as well.

Also, we might want to consider a normalize() or canonicalize() method, which for GDF_BOOL would replace non-zero values with 1's and keep 0's as-is.

So we'd add a normalize() call around all of the wrappers inside the various operators, and it'd just be a pass-through for all but gdf_bool?

eyalroz · 2019-02-01T19:07:02Z

So we'd add a normalize() call around all of the wrappers inside the various operators, and it'd just be a pass-through for all but gdf_bool?

If by pass-through you mean it would do nothing and be noexcept and inline, then yes.

harrism · 2019-02-12T22:41:44Z

@jrhemstad @BradReesWork What needs to happen with this PR?

eyalroz · 2019-02-22T15:27:28Z

@harrism

One thing I hope would be considered (whether rejected or accepted) is my typedef int8_t gdf_bool; suggestion. This would be useful, in particular, for the apply_stencil reimplementation and also for consistency IMHO.

jrhemstad · 2019-02-22T16:03:25Z

@harrism

One thing I hope would be considered (whether rejected or accepted) is my typedef int8_t gdf_bool; suggestion. This would be useful, in particular, for the apply_stencil reimplementation and also for consistency IMHO.

I 100% support this as soon as #599 is completed. Otherwise, the CFFI will fail due to inability to parse the required #include <stdint.h>

eyalroz · 2019-02-22T16:50:55Z

@jrhemstad :

I 100% support this as soon as #599 is completed. Otherwise, the CFFI will fail due to inability to parse the required #include <stdint.h>

In the mean time, we could use this hack:

#ifndef _STDINT_H
typedef signed char        int8_t;
#endif
typedef int8_t             gdf_bool;

or even simply

typedef signed char  gdf_bool;

and cross our fingers (or require with CMake, or static_assert somewhere) that they're the same type.

eyalroz · 2019-03-06T20:04:33Z

@BradReesWork , @harrism : Pinging you guys about this PR, because it has a slight (compatible) overlap with my PR #902 , and if it goes forward I'll base myself on what's gone in.

jrhemstad · 2019-03-07T22:28:55Z

Superseded by #1142

harrism · 2019-03-07T22:50:37Z

@jrhemstad is going to take over this PR. We'll move it to 0.7.

BradReesWork added 3 commits January 30, 2019 11:46

added Boolean type

7ddf2ee

added bool type

afccdd6

added PR 817

8b974d9

BradReesWork requested review from jrhemstad and harrism January 30, 2019 22:02

jrhemstad requested a review from eyalroz January 30, 2019 22:20

jrhemstad reviewed Jan 30, 2019

View reviewed changes

cpp/src/utilities/wrapper_types.hpp Show resolved Hide resolved

eyalroz reviewed Jan 31, 2019

View reviewed changes

jrhemstad requested changes Jan 31, 2019

View reviewed changes

kkraus14 added 0 - Waiting on Author Waiting for author to respond to review libcudf Affects libcudf (C++/CUDA) code. labels Feb 11, 2019

harrism assigned BradReesWork Feb 12, 2019

Merge branch 'branch-0.6' into branch-0.6

befacf0

jrhemstad mentioned this pull request Mar 7, 2019

[REVIEW] Add GDF_BOOL type #1142

Merged

12 tasks

jrhemstad closed this Mar 7, 2019

harrism assigned jrhemstad and unassigned BradReesWork Mar 7, 2019

jrhemstad mentioned this pull request Jun 5, 2019

Add bool8(bool) constructor #1932

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[REVIEW] Added GDF_BOOL type to cuda code #817

[REVIEW] Added GDF_BOOL type to cuda code #817

BradReesWork commented Jan 30, 2019

eyalroz left a comment

eyalroz Jan 31, 2019

BradReesWork Feb 25, 2019

jrhemstad Feb 25, 2019 •

edited

Loading

eyalroz Feb 25, 2019

jrhemstad Feb 25, 2019

eyalroz Jan 31, 2019

eyalroz Jan 31, 2019

jrhemstad Jan 31, 2019

eyalroz Jan 31, 2019 •

edited

Loading

BradReesWork Feb 25, 2019

eyalroz Feb 25, 2019

eyalroz Jan 31, 2019

BradReesWork Feb 25, 2019

jrhemstad commented Jan 31, 2019 •

edited

Loading

jrhemstad left a comment

kkraus14 commented Jan 31, 2019

eyalroz commented Feb 1, 2019

jrhemstad commented Feb 1, 2019

eyalroz commented Feb 1, 2019

harrism commented Feb 12, 2019

eyalroz commented Feb 22, 2019

jrhemstad commented Feb 22, 2019

eyalroz commented Feb 22, 2019 •

edited

Loading

eyalroz commented Mar 6, 2019

jrhemstad commented Mar 7, 2019

harrism commented Mar 7, 2019

		@@ -263,6 +263,9 @@ using date32 = detail::wrapper<gdf_date32, GDF_DATE32>;

		using date64 = detail::wrapper<gdf_date64, GDF_DATE64>;

		using gdf_bool = detail::wrapper<int8_t, GDF_BOOL>;

[REVIEW] Added GDF_BOOL type to cuda code #817

[REVIEW] Added GDF_BOOL type to cuda code #817

Conversation

BradReesWork commented Jan 30, 2019

eyalroz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrhemstad Feb 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eyalroz Jan 31, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrhemstad commented Jan 31, 2019 • edited Loading

jrhemstad left a comment

Choose a reason for hiding this comment

kkraus14 commented Jan 31, 2019

eyalroz commented Feb 1, 2019

jrhemstad commented Feb 1, 2019

eyalroz commented Feb 1, 2019

harrism commented Feb 12, 2019

eyalroz commented Feb 22, 2019

jrhemstad commented Feb 22, 2019

eyalroz commented Feb 22, 2019 • edited Loading

eyalroz commented Mar 6, 2019

jrhemstad commented Mar 7, 2019

harrism commented Mar 7, 2019

jrhemstad Feb 25, 2019 •

edited

Loading

eyalroz Jan 31, 2019 •

edited

Loading

jrhemstad commented Jan 31, 2019 •

edited

Loading

eyalroz commented Feb 22, 2019 •

edited

Loading