Introduce dispatching in oneAPI covariance algorithm #2527

Vika-F · 2023-09-26T08:52:35Z

Only batch algorithm was modified for now. Online part of the covariance algorithm will be modified further in a separate PR.

Changes proposed in this pull request:

Added computre_parameters_cpu and computre_parameters_gpu classes in covariance algorithm. Those classes encapsulate the dispatching functionality for CPU and GPU respectively.
Added test for dispatching functionality in covariance algorithm.

… in Covariance algorithm

…pute_parameters_impl classes

Pull changes from master

Vika-F · 2023-10-05T10:50:00Z

/intelci: run

….cpp

Vika-F · 2023-10-09T09:27:35Z

/intelci: run

inteldimitrius

LGTM

ahuber21

Just a few points for discussion and a request for more tests. Very interesting work!

ahuber21 · 2023-10-10T08:46:19Z

cpp/oneapi/dal/algo/covariance/backend/cpu/compute_kernel_dense.cpp

@@ -35,9 +36,24 @@ template <typename Float, daal::CpuType Cpu>
 using daal_covariance_kernel_t = daal_covariance::internal::
    CovarianceDenseBatchKernel<Float, daal_covariance::Method::defaultDense, Cpu>;

+template <typename Float, typename Task>
+static auto convert_parameters(const detail::compute_parameters<Task>& params) {


I wouldn't use auto as return type

It is not a user facing function

cpp/daal/include/algorithms/algorithm_base_mode.h

cpp/oneapi/dal/algo/covariance/backend/cpu/compute_kernel_dense.cpp

cpp/oneapi/dal/algo/covariance/backend/cpu/BUILD

cpp/oneapi/dal/algo/covariance/compute_types.cpp

ahuber21 · 2023-10-10T09:01:50Z

cpp/oneapi/dal/algo/covariance/detail/compute_ops.cpp

+    inline auto implementation(const Policy& ctx,
+                               const descriptor_base<Task>& desc,
+                               const compute_parameters<Task>& params,
+                               const compute_input<Task>& input) const {
        using kernel_dispatcher_t = dal::backend::kernel_dispatcher< //


I see the same // to achieve a line break was already in the code ... hmm, is this really the best way to do it?

cpp/oneapi/dal/algo/covariance/detail/compute_ops.hpp

cpp/oneapi/dal/algo/covariance/parameters/cpu/compute_parameters.cpp

Vika-F · 2023-10-11T11:25:20Z

/intelci: run

ahuber21

Thanks for addressing my comments! I don't want to block progress here any longer.

KulikovNikita · 2023-10-10T11:48:22Z

cpp/oneapi/dal/algo/covariance/backend/cpu/compute_kernel_dense.cpp

@@ -35,9 +36,24 @@ template <typename Float, daal::CpuType Cpu>
 using daal_covariance_kernel_t = daal_covariance::internal::
    CovarianceDenseBatchKernel<Float, daal_covariance::Method::defaultDense, Cpu>;

+template <typename Float, typename Task>
+static auto convert_parameters(const detail::compute_parameters<Task>& params) {


It is not a user facing function

cpp/oneapi/dal/algo/covariance/compute_types.cpp

KulikovNikita · 2023-10-12T07:19:11Z

cpp/oneapi/dal/algo/covariance/backend/cpu/compute_kernel_dense.cpp

+    auto status = daal_hyperparameter.set(HyperparameterId::denseUpdateStepBlockSize, block);
+    interop::status_to_exception(status);
+
+    return daal_hyperparameter;


Do we really want to pass it by value? What about using std::shared_ptr?

I think shared_ptr is too heavy for this case.
It's better to just assign the result to const reference -> no passing by value and the lifetime is acceptable.
The full description of the trick is in GotW #88

KulikovNikita · 2023-10-12T07:27:03Z

cpp/oneapi/dal/algo/covariance/parameters/cpu/compute_parameters.cpp

+        if (5000l < row_count && row_count <= 50000l) {
+            block_size = 1024l;
+        }


Do we want dispatching for avx2 as well?

This logic is replicated from DAAL. I think performance testing needed here to be sure that the change gives some benefit on AVX2.

I think this is outside of the scope of this PR.

KulikovNikita · 2023-10-12T08:07:17Z

cpp/oneapi/dal/algo/covariance/parameters/gpu/compute_parameters_dpc.cpp

+    params_t operator()(const context_gpu& ctx,
+                        const detail::descriptor_base<Task>& desc,
+                        const compute_input<Task>& input) const {
+        return params_t{};


Do we have blocking here?

I think no, at least for now. I do not see any blocking here:
https://github.com/oneapi-src/oneDAL/blob/master/cpp/oneapi/dal/backend/primitives/stat/cov_dpc.cpp#L55
and here:
https://github.com/oneapi-src/oneDAL/blob/master/cpp/oneapi/dal/algo/covariance/backend/gpu/compute_kernel_dense_impl_dpc.cpp#L120

KulikovNikita · 2023-10-12T08:09:00Z

cpp/oneapi/dal/algo/covariance/test/compute_parameters.cpp

+        GENERATE_DATAFRAME(te::dataframe_builder{ 1000, 20 }.fill_uniform(-30, 30, 7777),
+                           te::dataframe_builder{ 100, 10 }.fill_uniform(0, 1, 7777),
+                           te::dataframe_builder{ 100, 10 }.fill_uniform(-10, 10, 7777),
+                           te::dataframe_builder{ 500, 40 }.fill_uniform(-100, 100, 7777),
+                           te::dataframe_builder{ 500, 250 }.fill_uniform(0, 1, 7777));


I think you want to have some non-trivial dispatch results. Please remove really small tests and extend with tests designed for wide and/or long datasets

Vika-F · 2023-10-12T10:39:10Z

/intelci: run

Vika-F added 8 commits September 20, 2023 09:50

Initial implementation of hyperparameters passing from oneDAL to DAAL…

d23b0b4

… in Covariance algorithm

Fix a typo

038ea2a

Fix build errors and typos

82aba4a

Add test for compute parameters in covariance algorithm

b219754

Partial fix in Covariance tests bazel build

afcaa66

Partial fix in Covariance tests bazel build

2eee7f8

Add definition for covariance::detail::v1::compute_parameters and com…

061af10

…pute_parameters_impl classes

Merge pull request #11 from oneapi-src/master

ca47338

Pull changes from master

Vika-F added enhancement dpc++ Issue/PR related to DPC++ functionality labels Oct 4, 2023

Fix a typo 'float_t' -> 'Float' in covariance/test/compute_parameters…

ec73b4d

….cpp

Vika-F marked this pull request as ready for review October 10, 2023 08:09

Vika-F requested review from Alexsandruss, samir-nasibli and KulikovNikita as code owners October 10, 2023 08:09

Vika-F requested a review from Alexandr-Solovev October 10, 2023 08:09

inteldimitrius approved these changes Oct 10, 2023

View reviewed changes

ahuber21 reviewed Oct 10, 2023

View reviewed changes

Vika-F added 2 commits October 11, 2023 01:47

Apply review comments

23cb17c

Add hyperparameters ranges checks

6742d47

ahuber21 approved these changes Oct 12, 2023

View reviewed changes

Alexandr-Solovev approved these changes Oct 12, 2023

View reviewed changes

KulikovNikita reviewed Oct 12, 2023

View reviewed changes

Apply review comments

2c4d121

Vika-F merged commit 85ad140 into oneapi-src:master Oct 17, 2023
14 checks passed

Vika-F deleted the dev/vsfedoto/cov_dispatcher branch January 17, 2024 10:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce dispatching in oneAPI covariance algorithm #2527

Introduce dispatching in oneAPI covariance algorithm #2527

Vika-F commented Sep 26, 2023 •

edited

Loading

Vika-F commented Oct 5, 2023

Vika-F commented Oct 9, 2023

inteldimitrius left a comment

ahuber21 left a comment

ahuber21 Oct 10, 2023

KulikovNikita Oct 10, 2023

ahuber21 Oct 10, 2023

Vika-F commented Oct 11, 2023

ahuber21 left a comment

KulikovNikita Oct 10, 2023

KulikovNikita Oct 12, 2023

Vika-F Oct 12, 2023

KulikovNikita Oct 12, 2023

Vika-F Oct 12, 2023

KulikovNikita Oct 12, 2023

Vika-F Oct 12, 2023

KulikovNikita Oct 12, 2023

Vika-F Oct 12, 2023

Vika-F commented Oct 12, 2023

Introduce dispatching in oneAPI covariance algorithm #2527

Introduce dispatching in oneAPI covariance algorithm #2527

Conversation

Vika-F commented Sep 26, 2023 • edited Loading

Vika-F commented Oct 5, 2023

Vika-F commented Oct 9, 2023

inteldimitrius left a comment

Choose a reason for hiding this comment

ahuber21 left a 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

Vika-F commented Oct 11, 2023

ahuber21 left a 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

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

Vika-F commented Oct 12, 2023

Vika-F commented Sep 26, 2023 •

edited

Loading