WIP: add bool type

[tylerjereddy]

• 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]

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

[tylerjereddy]

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]

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]

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
  }

[tylerjereddy]

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]

@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]

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]

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]

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]

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]

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]

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