Applying review comments

Author: AlexanderKalistratov

dpnp/backend/extensions/statistics/bincount.hpp

@@ -25,10 +25,11 @@
 
 #pragma once
 
-#include <dpctl4pybind11.hpp>
+#include <pybind11/pybind11.h>
 #include <sycl/sycl.hpp>
 
 #include "dispatch_table.hpp"
+#include "dpctl4pybind11.hpp"
 
 namespace dpctl_td_ns = dpctl::tensor::type_dispatch;
 

dpnp/backend/extensions/statistics/common.hpp

@@ -28,33 +28,10 @@
 #include <complex>
 #include <pybind11/numpy.h>
 #include <pybind11/pybind11.h>
-
-// clang-format off
-// math_utils.hpp doesn't include sycl header but uses sycl types
-// so sycl.hpp must be included before math_utils.hpp
 #include <sycl/sycl.hpp>
+
 #include "utils/math_utils.hpp"
 #include "utils/type_utils.hpp"
-// clang-format on
-
-namespace dpctl
-{
-namespace tensor
-{
-namespace type_utils
-{
-// Upstream to dpctl
-template <class T>
-struct is_complex<const std::complex<T>> : std::true_type
-{
-};
-
-template <typename T>
-constexpr bool is_complex_v = is_complex<T>::value;
-
-} // namespace type_utils
-} // namespace tensor
-} // namespace dpctl
 
 namespace type_utils = dpctl::tensor::type_utils;
 
@@ -115,18 +92,16 @@ struct IsNan
     static bool isnan(const T &v)
     {
         if constexpr (type_utils::is_complex_v<T>) {
-            const auto real1 = std::real(v);
-            const auto imag1 = std::imag(v);
-
             using vT = typename T::value_type;
 
+            const vT real1 = std::real(v);
+            const vT imag1 = std::imag(v);
+
             return IsNan<vT>::isnan(real1) || IsNan<vT>::isnan(imag1);
         }
-        else {
-            if constexpr (std::is_floating_point_v<T> ||
-                          std::is_same_v<T, sycl::half>) {
-                return sycl::isnan(v);
-            }
+        else if constexpr (std::is_floating_point_v<T> ||
+                           std::is_same_v<T, sycl::half>) {
+            return sycl::isnan(v);
         }
 
         return false;

dpnp/backend/extensions/statistics/histogram.hpp

@@ -25,9 +25,11 @@
 
 #pragma once
 
+#include <pybind11/pybind11.h>
 #include <sycl/sycl.hpp>
 
 #include "dispatch_table.hpp"
+#include "dpctl4pybind11.hpp"
 
 namespace dpctl_td_ns = dpctl::tensor::type_dispatch;
 

dpnp/backend/extensions/statistics/histogram_common.hpp

@@ -74,13 +74,13 @@ struct CachedData
     template <int _Dims>
     void init(const sycl::nd_item<_Dims> &item) const
     {
-        int32_t llid = item.get_local_linear_id();
+        uint32_t llid = item.get_local_linear_id();
         auto local_ptr = &local_data[0];
-        int32_t size = local_data.size();
+        uint32_t size = local_data.size();
         auto group = item.get_group();
-        int32_t local_size = group.get_local_linear_range();
+        uint32_t local_size = group.get_local_linear_range();
 
-        for (int32_t i = llid; i < size; i += local_size) {
+        for (uint32_t i = llid; i < size; i += local_size) {
             local_ptr[i] = global_data[i];
         }
     }
@@ -218,15 +218,15 @@ struct HistWithLocalCopies
     template <int _Dims>
     void finalize(const sycl::nd_item<_Dims> &item) const
     {
-        int32_t llid = item.get_local_linear_id();
-        int32_t bins_count = local_hist.get_range().get(1);
-        int32_t local_hist_count = local_hist.get_range().get(0);
+        uint32_t llid = item.get_local_linear_id();
+        uint32_t bins_count = local_hist.get_range().get(1);
+        uint32_t local_hist_count = local_hist.get_range().get(0);
         auto group = item.get_group();
-        int32_t local_size = group.get_local_linear_range();
+        uint32_t local_size = group.get_local_linear_range();
 
-        for (int32_t i = llid; i < bins_count; i += local_size) {
+        for (uint32_t i = llid; i < bins_count; i += local_size) {
             auto value = local_hist[0][i];
-            for (int32_t lhc = 1; lhc < local_hist_count; ++lhc) {
+            for (uint32_t lhc = 1; lhc < local_hist_count; ++lhc) {
                 value += local_hist[lhc][i];
             }
             if (value != T(0)) {

dpnp/backend/extensions/statistics/histogramdd.cpp

@@ -245,7 +245,7 @@ struct HistogramddF
 };
 
 template <typename T, typename HistType = size_t>
-using HistogramddF2 = HistogramddF<T, T, HistType>;
+using HistogramddF_ = HistogramddF<T, T, HistType>;
 
 using SupportedTypes =
     std::tuple<std::tuple<uint64_t, float>,
@@ -268,7 +268,7 @@ using SupportedTypes =
 
 Histogramdd::Histogramdd() : dispatch_table("sample", "histogram")
 {
-    dispatch_table.populate_dispatch_table<SupportedTypes, HistogramddF2>();
+    dispatch_table.populate_dispatch_table<SupportedTypes, HistogramddF_>();
 }
 
 std::tuple<sycl::event, sycl::event> Histogramdd::call(

dpnp/backend/extensions/statistics/histogramdd.hpp

@@ -25,12 +25,11 @@
 
 #pragma once
 
+#include <pybind11/pybind11.h>
 #include <sycl/sycl.hpp>
 
-// dpctl tensor headers
-#include "dpctl4pybind11.hpp"
-
 #include "dispatch_table.hpp"
+#include "dpctl4pybind11.hpp"
 
 namespace statistics
 {

dpnp/dpnp_iface_histograms.py

@@ -235,32 +235,6 @@ def _get_bin_edges(a, bins, range, usm_type):
     return bin_edges, None
 
 
-def _normalize_array(a, dtype, usm_type=None):
-    if usm_type is None:
-        usm_type = a.usm_type
-
-    try:
-        return dpnp.asarray(
-            a,
-            dtype=dtype,
-            usm_type=usm_type,
-            sycl_queue=a.sycl_queue,
-            order="C",
-            copy=False,
-        )
-    except ValueError:
-        pass
-
-    return dpnp.asarray(
-        a,
-        dtype=dtype,
-        usm_type=usm_type,
-        sycl_queue=a.sycl_queue,
-        order="C",
-        copy=True,
-    )
-
-
 def _bincount_validate(x, weights, minlength):
     if x.ndim > 1:
         raise ValueError("object too deep for desired array")
@@ -426,16 +400,16 @@ def bincount(x, weights=None, minlength=None):
             "supported types"
         )
 
-    x_casted = _normalize_array(x, dtype=x_casted_dtype)
+    x_casted = dpnp.asarray(x, dtype=x_casted_dtype, order="C")
 
     if weights is not None:
-        weights_casted = _normalize_array(weights, dtype=ntype_casted)
+        weights_casted = dpnp.asarray(weights, dtype=ntype_casted, order="C")
 
     n_casted = _bincount_run_native(
         x_casted, weights_casted, minlength, ntype_casted, usm_type
     )
 
-    n = _normalize_array(n_casted, dtype=ntype, usm_type=usm_type)
+    n = dpnp.asarray(n_casted, dtype=ntype, usm_type=usm_type, order="C")
 
     return n
 
@@ -643,10 +617,12 @@ def histogram(a, bins=10, range=None, density=None, weights=None):
             "supported types"
         )
 
-    a_casted = _normalize_array(a, a_bin_dtype)
-    bin_edges_casted = _normalize_array(bin_edges, a_bin_dtype)
+    a_casted = dpnp.asarray(a, dtype=a_bin_dtype, order="C")
+    bin_edges_casted = dpnp.asarray(bin_edges, dtype=a_bin_dtype, order="C")
     weights_casted = (
-        _normalize_array(weights, hist_dtype) if weights is not None else None
+        dpnp.asarray(weights, dtype=hist_dtype, order="C")
+        if weights is not None
+        else None
     )
 
     # histogram implementation uses atomics, but atomics doesn't work with
@@ -681,7 +657,7 @@ def histogram(a, bins=10, range=None, density=None, weights=None):
     )
     _manager.add_event_pair(mem_ev, ht_ev)
 
-    n = _normalize_array(n_casted, dtype=ntype, usm_type=usm_type)
+    n = dpnp.asarray(n_casted, dtype=ntype, usm_type=usm_type, order="C")
 
     if density:
         db = dpnp.astype(
@@ -794,17 +770,9 @@ def _histdd_validate_bins(bins):
             )
 
 
-def _histdd_check_monotonicity(bins):
-    for i, b in enumerate(bins):
-        if dpnp.any(b[:-1] > b[1:]):
-            raise ValueError(
-                f"bins[{i}] must increase monotonically, when an array"
-            )
-
-
 def _histdd_normalize_bins(bins, ndims):
     if not isinstance(bins, Iterable):
-        if not isinstance(bins, int):
+        if not dpnp.issubdtype(type(bins), dpnp.integer):
             raise ValueError("'bins' must be an integer, when a scalar")
 
         bins = [bins] * ndims
@@ -1053,11 +1021,11 @@ def histogramdd(sample, bins=10, range=None, weights=None, density=False):
         bin_edges_list, edges_count_list, sample_dtype
     )
 
-    _histdd_check_monotonicity(bin_edges_view_list)
-
-    sample_ = _normalize_array(sample, sample_dtype)
+    sample_ = dpnp.asarray(sample, dtype=sample_dtype, order="C")
     weights_ = (
-        _normalize_array(weights, hist_dtype) if weights is not None else None
+        dpnp.asarray(weights, dtype=hist_dtype, order="C")
+        if weights is not None
+        else None
     )
     n = _histdd_run_native(
         sample_,
@@ -1069,7 +1037,7 @@ def histogramdd(sample, bins=10, range=None, weights=None, density=False):
     )
 
     expexted_hist_dtype = _histdd_hist_dtype(queue, weights)
-    n = _normalize_array(n, expexted_hist_dtype, usm_type)
+    n = dpnp.asarray(n, dtype=expexted_hist_dtype, usm_type=usm_type, order="C")
 
     if density:
         # calculate the probability density function

dpnp/tests/test_histogram.py

@@ -642,10 +642,15 @@ def test_linspace_data(self, dtype):
         assert_array_equal(result_hist, expected_hist)
 
     @pytest.mark.parametrize("xp", [numpy, dpnp])
-    def test_invalid_bin(self, xp):
+    def test_invalid_bin_float(self, xp):
         a = xp.array([[1, 2]])
         assert_raises(ValueError, xp.histogramdd, a, bins=0.1)
 
+    @pytest.mark.parametrize("xp", [numpy, dpnp])
+    def test_invalid_bin_2d_array(self, xp):
+        a = xp.array([[1, 2]])
+        assert_raises(ValueError, xp.histogramdd, a, bins=[[[10]], 10])
+
     @pytest.mark.parametrize(
         "bins",
         [
@@ -730,7 +735,7 @@ def test_infinite_edge(self, xp, inf_val):
     def test_unsigned_monotonicity_check(self, xp):
         # bins must increase monotonically when bins contain unsigned values
         arr = xp.array([2])
-        bins = xp.array([1, 3, 1], dtype="uint64")
+        bins = [xp.array([1, 3, 1], dtype="uint64")]
         with assert_raises(ValueError):
             xp.histogramdd(arr, bins=bins)