WIP: add bool type #54

tylerjereddy · 2022-10-25T22:23:39Z

I need a bool type for Python array API standard
conformance, and as far as I know Kokkos doesn't work
with such a view type under the hood?
if I alias bool_ and bool to the uint8 type,
the conformance test suite can see through it, and
resolves the alias and fails my suite
I'm trying another approach now, but this fails
i.e., the local pykokkos-base testsuite with

AttributeError: module 'kokkos.libpykokkos' has no attribute 'KokkosView_bool_HostSpace_LayoutRight_1'

I'm not so sure either of these approaches really have many
prospects for success--I'm open to better ideas...

[skip ci]

* I need a bool type for Python array API standard conformance, and as far as I know Kokkos doesn't work with such a view type under the hood? * if I alias `bool_` and `bool` to the `uint8` type, the conformance test suite can see through it, and resolves the alias and fails my suite * I'm trying another approach now, but this fails i.e., the local `pykokkos-base` testsuite with ``` AttributeError: module 'kokkos.libpykokkos' has no attribute 'KokkosView_bool_HostSpace_LayoutRight_1' ``` * I'm not so sure either of these approaches really have many prospects for success--I'm open to better ideas... [skip ci]

tylerjereddy · 2022-10-25T22:24:07Z

include/fwd.hpp

@@ -134,6 +134,7 @@ enum KokkosViewDataType {
  Uint64,
  Float32,
  Float64,
+  Bool,


doesn't this Bool entity need to actually exist under the hood??

not for the enum, this basically is just a definition

JBludau · 2022-10-26T01:16:17Z

include/traits.hpp

+typedef uint8_t bool_t;
+


please if you do use using bool_t = uint8_t

JBludau · 2022-10-26T01:17:17Z

include/fwd.hpp

@@ -134,6 +134,7 @@ enum KokkosViewDataType {
  Uint64,
  Float32,
  Float64,
+  Bool,


not for the enum, this basically is just a definition

tylerjereddy · 2022-10-26T15:39:26Z

Based on feedback from Jakob, I pushed in a small change to switch back to compiling with genuine bool type (I think), which does succeed at build stage, but has a few tests not passing yet:

FAILED kokkos/test/views.py::PyKokkosBaseViewsTests::test_view_access - AssertionError: True != 2
FAILED kokkos/test/views.py::PyKokkosBaseViewsTests::test_view_create_mirror - AssertionError: True != 3
FAILED kokkos/test/views.py::PyKokkosBaseViewsTests::test_view_create_mirror_view - AssertionError: True != 3
FAILED kokkos/test/views.py::PyKokkosBaseViewsTests::test_view_deep_copy - AssertionError: True != 3
FAILED kokkos/test/views.py::PyKokkosBaseViewsTests::test_view_iadd - AssertionError: True != 4
FAILED kokkos/test/views.py::PyKokkosBaseViewsTests::test_view_imul - AssertionError: True != 3
FAILED kokkos/test/views.py::PyKokkosBaseViewsTests::test_view_isub - AssertionError: True != 7

Each of those failures look pretty similar--one example:

__________________________________________________________________________________________________________________________________________ PyKokkosBaseViewsTests.test_view_isub ___________________________________________________________________________________________________________________________________________

self = <test.views.PyKokkosBaseViewsTests testMethod=test_view_isub>

    def test_view_isub(self):
        """view_isub"""
    
        print("")
        for itr in conf.get_variants({"memory_traits": [kokkos.Atomic]}):
            _shape = itr[0]
            _idx = itr[1]
            _zeros = itr[2]
            _kwargs = itr[3]
    
            _data = conf.generate_variant(_shape, **_kwargs)
    
            self._print_info(_data)
    
            _host = [_data[0].create_mirror_view(), _data[1].create_mirror_view()]
    
            _host[0][_idx] = 10
            _host[1][_idx] = 20
    
            _host[0][_idx] -= 3
            _host[1][_idx] -= 3
    
            _data[0].deep_copy(_host[0])
            _data[1].deep_copy(_host[1])
    
            self.assertEqual(_data[0].create_mirror_view()[_zeros], 0)
            self.assertEqual(_data[1].create_mirror_view()[_zeros], 0)
>           self.assertEqual(_data[0].create_mirror_view()[_idx], 7)
E           AssertionError: True != 7

kokkos/test/views.py:202: AssertionError

* compile using genuine `bool` type instead of an alias * this seems to succeed at build stage, but a few tests do fail at the moment [skip ci]

* to accommodate `bool` view type, we use `True` instead of actual integer values in a number of tests

tylerjereddy · 2022-10-26T21:58:26Z

Ok, tests are passing locally now with the "native" bool view type--we'll see if the CI agrees. If it does, please do check the testing changes carefully.

I think they make sense--basically any positive non-zero integer value should be considered True for testing purposes.

tylerjereddy · 2022-10-26T22:03:30Z

Looks like a subset of CI CMake builds fail with: kokkos/core/src/impl/Kokkos_Atomic_View.hpp:218:73: error: ‘*’ in boolean context, suggest ‘&&’ instead [-Werror=int-in-bool-context]

jrmadsen · 2022-10-26T22:32:11Z

Easy fix, in include/variants/atomics.hpp:

Exclude all meaningless/invalid arithmetic operations on booleans via an if constexpr, e.g.

  _atomic.def(
      "__mul__", [](atomic_type _obj, value_type _v) { return (_obj * _v); },
      py::is_operator());

should be:

  constexpr bool is_boolean = std::is_same<Tp, bool>::value;

  if constexpr(!is_boolean) {
    _atomic.def(
        "__mul__", [](atomic_type _obj, value_type _v) { return (_obj * _v); },
        py::is_operator());

    // ... etc
  }

jrmadsen · 2022-10-26T22:40:27Z

kokkos/test/views.py

+            if _kwargs["dtype"] == kokkos.bool:
+                self.assertEqual(_data[1].create_mirror_view()[_idx], True)
+            else:
+                self.assertEqual(_data[1].create_mirror_view()[_idx], 2)


Suggested change

if _kwargs["dtype"] == kokkos.bool:

self.assertEqual(_data[1].create_mirror_view()[_idx], True)

else:

self.assertEqual(_data[1].create_mirror_view()[_idx], 2)

value = lambda v : v if _kwargs["dtype"] != kokkos.bool else v > 0

self.assertEqual(_data[1].create_mirror_view()[_idx], value(2))

Just thought this scheme would be easier to maintain + more compact

tylerjereddy · 2022-10-26T23:09:32Z

I think __mul__ should be allowed for bool though:

>>> import numpy as np
>>> np.array([True, True]) * np.array([False, False])
array([False, False])

If C++ doesn't allow it, I suppose we could disable it here and use casting in the ufunc inner loops in pykokkos proper.

jrmadsen · 2022-10-27T17:15:02Z

@tylerjereddy sorry, disregard that statement. I saw something suggesting the compiler was trying to use atomic_fetch_add and atomic_fetch_sub and since the std::atomic docs on cppreference don't list any overloads for multiplication or division, I came to a wrong conclusion. C++ does allow multiplying bools and from the looks of your CI, the kokkos equivalent of the numpy multiplication you provided above is supported and passing -- the analogue to your failures would be something like np.array([True, True], dtype=np.atomicBool) if numpy supported atomic booleans.

jrmadsen · 2022-10-27T17:29:54Z

I don't think this is necessarily an error. It is failing to build bc of -Werror=int-in-bool-context, i.e. you are using an integer value with a bool and allowing it to behave implicitly like a bool -- where any nonzero value is true. This might be valid in this context. I'd consult with Christian or Damien and see if they are fine with doing:

#if defined(__GNUC__)  // GCC and clang
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wint-in-bool-context"
#endif

// affected regions within generate_atomic_variant

#if defined(__GNUC__)  // GCC and clang
#pragma GCC diagnostic pop
#endif

JBludau · 2022-10-27T17:46:56Z

Maybe I don't understand, but what would a multiplication of bools look like? I guess if you want to do this in cpp it will implicitly cast it to int, multiply and then cast it back ... which is the same as doing && for bool which is more expressive and actually well defined ... which is exactly what the warning suggests to do.
I think we should not pragma push the flag but provide correct specializations for bool as a type. Meaning having a specialized impl. that uses the &&

JBludau · 2022-10-27T17:52:53Z

And if this is not possible, why not static_cast<bool>(myint) where necessary? this would be expressive at least and should disable the warning

jrmadsen · 2022-10-27T19:58:48Z

And if this is not possible, why not static_cast<bool>(myint) where necessary? this would be expressive at least and should disable the warning

This would be ideal, I was under the impression from looking at the lambdas that atomic_type and the value_type were bools and the conversion to an int was happening in the operator overload.

jrmadsen · 2022-10-27T20:03:27Z

Maybe I don't understand, but what would a multiplication of bools look like? I guess if you want to do this in cpp it will implicitly cast it to int, multiply and then cast it back ... which is the same as doing && for bool which is more expressive and actually well defined ... which is exactly what the warning suggests to do.
I think we should not pragma push the flag but provide correct specializations for bool as a type. Meaning having a specialized impl. that uses the &&

Agree in principal but you would need a partial specialization and this is a freestanding function

jrmadsen · 2022-10-27T20:04:41Z

Oh wait nevermind, I remember seeing C++17 or higher is a requirement for Kokkos now so an if constexpr would work.

tylerjereddy commented Oct 25, 2022

View reviewed changes

JBludau reviewed Oct 26, 2022

View reviewed changes

MAINT: PR 54 revisions

d5c85c6

* compile using genuine `bool` type instead of an alias * this seems to succeed at build stage, but a few tests do fail at the moment [skip ci]

tylerjereddy force-pushed the treddy_bool_alias_draft branch from f5d66e6 to d5c85c6 Compare October 26, 2022 15:41

MAINT: PR 54 revisions

39f1f41

* to accommodate `bool` view type, we use `True` instead of actual integer values in a number of tests

jrmadsen reviewed Oct 26, 2022

View reviewed changes

tylerjereddy changed the title ~~WIP: add bool type (alias)~~ WIP: add bool type Oct 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: add bool type #54

WIP: add bool type #54

tylerjereddy commented Oct 25, 2022

tylerjereddy Oct 25, 2022

JBludau Oct 26, 2022

JBludau Oct 26, 2022

JBludau Oct 26, 2022

tylerjereddy commented Oct 26, 2022

tylerjereddy commented Oct 26, 2022 •

edited

Loading

tylerjereddy commented Oct 26, 2022

jrmadsen commented Oct 26, 2022

jrmadsen Oct 26, 2022 •

edited

Loading

tylerjereddy commented Oct 26, 2022

jrmadsen commented Oct 27, 2022 •

edited

Loading

jrmadsen commented Oct 27, 2022

JBludau commented Oct 27, 2022 •

edited

Loading

JBludau commented Oct 27, 2022

jrmadsen commented Oct 27, 2022

jrmadsen commented Oct 27, 2022

jrmadsen commented Oct 27, 2022

WIP: add bool type #54

Are you sure you want to change the base?

WIP: add bool type #54

Conversation

tylerjereddy commented Oct 25, 2022

tylerjereddy Oct 25, 2022

Choose a reason for hiding this comment

JBludau Oct 26, 2022

Choose a reason for hiding this comment

JBludau Oct 26, 2022

Choose a reason for hiding this comment

JBludau Oct 26, 2022

Choose a reason for hiding this comment

tylerjereddy commented Oct 26, 2022

tylerjereddy commented Oct 26, 2022 • edited Loading

tylerjereddy commented Oct 26, 2022

jrmadsen commented Oct 26, 2022

jrmadsen Oct 26, 2022 • edited Loading

Choose a reason for hiding this comment

tylerjereddy commented Oct 26, 2022

jrmadsen commented Oct 27, 2022 • edited Loading

jrmadsen commented Oct 27, 2022

JBludau commented Oct 27, 2022 • edited Loading

JBludau commented Oct 27, 2022

jrmadsen commented Oct 27, 2022

jrmadsen commented Oct 27, 2022

jrmadsen commented Oct 27, 2022

tylerjereddy commented Oct 26, 2022 •

edited

Loading

jrmadsen Oct 26, 2022 •

edited

Loading

jrmadsen commented Oct 27, 2022 •

edited

Loading

JBludau commented Oct 27, 2022 •

edited

Loading