Copy device_matrix_data into arrays #1476

MarcelKoch · 2023-11-28T09:57:35Z

This PR changes the behavior for the read(const device_matrix_data&) overload for Coo and Csr.

Currently, the const & read overload copies the input device_matrix_data and moves it into the && overload. The && overload will cause the internal arrays of the matrix to use the arrays of the device_matrix_data. This means that if the matrix was created from views, the matrix after the read will have owning arrays.

For the && overload this is justified, since here the matrix takes ownership of the input data. But for the const & it's unexpected, since no ownership is transferred. So, this PR makes the const & copy the input device matrix data directly into the matrix's arrays. The ownership property of the internal arrays will thus be preserved.

Note: The behavior of the && overload is a bit inconsistent across our matrix types. For Coo and Csr the && overload will result in the matrix arrays having ownership. But for all other matrix types, the ownership property is not changed. Maybe we should consolidate that. I was under the impression that device_matrix_data always owns its arrays. But that is not the case. The device_matrix_data can also be constructed from array, which might be views.

MarcelKoch · 2023-11-28T09:58:41Z

This fixes an issue in #1450

core/matrix/csr.cpp

core/matrix/coo.cpp

core/matrix/csr.cpp

include/ginkgo/core/base/array.hpp

upsj

LGTM!

include/ginkgo/core/base/exception_helpers.hpp

MarcelKoch · 2023-11-28T16:01:39Z

format!

MarcelKoch · 2023-11-30T11:13:16Z

Another question: how should const matrix_data& be handled. Currently it calls device_matrix_data&& internally, so it moves into the stored arrays. Should that stay, or also call const device_matrix_data&?

upsj · 2023-11-30T12:09:32Z

This only applies to Csr and Coo only, right? In that case, I would suggest calling aos_to_soa directly onto the resized arrays inside the matrix, i.e. combining the copy with the transformation

upsj

LGTM for the most part, only some test assertions are throwing instead of failing the test directly.

core/matrix/coo.cpp

core/matrix/csr.cpp

core/test/matrix/coo.cpp

core/test/matrix/csr.cpp

include/ginkgo/core/base/array.hpp

pratikvn

Macro change and usage of this->

core/matrix/coo.cpp

core/matrix/csr.cpp

include/ginkgo/core/base/exception_helpers.hpp

include/ginkgo/core/base/array.hpp

core/test/base/array.cpp

pratikvn · 2023-12-06T08:35:09Z

Also a general question: Is a macro part of the public interface and if so does removing a macro mean breaking interface ?

pratikvn

Thanks for updating this! LGTM!

upsj

LGTM! The macro is part of our public headers, but to me, it's only intended for internal usage, so I'm fine with removing it. There is no such thing as a detail macro after all.

include/ginkgo/core/base/array.hpp

yhmtsai

I do not think the device_matrix_data view means the matrix also be a view on it.
If user want the Coo view on the device_matrix_data view, they can use the Coo constructor rather than the read

include/ginkgo/core/base/array.hpp

core/matrix/coo.cpp

core/test/base/array.cpp

yhmtsai · 2023-12-07T10:10:20Z

core/test/matrix/coo.cpp

+    auto row_idxs = gko::array<index_type>(this->exec, 4);
+    auto col_idxs = gko::array<index_type>(this->exec, 4);
+    auto values = gko::array<value_type>(this->exec, 4);
+    auto m = Mtx::create(this->exec, gko::dim<2>{2, 3}, values.as_view(),
+                         col_idxs.as_view(), row_idxs.as_view());


if the view is larger than read size?

I will add tests to check that is throws.

core/test/matrix/csr.cpp

include/ginkgo/core/base/array.hpp

include/ginkgo/core/base/exception_helpers.hpp

yhmtsai · 2023-12-07T10:19:31Z

core/matrix/csr.cpp

+    // the device matrix data contains views on the column indices
+    // and values array of this matrix, and an owning array for the
+    // row indices (which doesn't exist in this matrix)


It introduces the inconsistence.

we had the same view + owning split previously when moving from a device_matrix_data, because row pointers need to be allocated separately in any case.

Yes, discussed something related to this with @MarcelKoch. My question is not introduced in this pr.
I will create another issue and we can discuss the reason there.

MarcelKoch · 2023-12-07T10:38:57Z

I do not think the device_matrix_data view means the matrix also be a view on it.
If user want the Coo view on the device_matrix_data view, they can use the Coo constructor rather than the read

I'm not understanding this. Do you want to see something changed?

yhmtsai · 2023-12-07T10:49:37Z

core/test/matrix/coo.cpp

+    ASSERT_EQ(orig_row_idxs, m->get_row_idxs());
+    ASSERT_EQ(orig_col_idxs, m->get_col_idxs());
+    ASSERT_EQ(orig_values, m->get_values());


matrix array view is now to use the device_data view not the original one

yes, but device_data is empty after the move, so we need to store the pointers beforehand.

yhmtsai · 2023-12-07T13:06:30Z

core/matrix/csr.cpp

+    // the device matrix data contains views on the column indices
+    // and values array of this matrix, and an owning array for the
+    // row indices (which doesn't exist in this matrix)


Yes, discussed something related to this with @MarcelKoch. My question is not introduced in this pr.
I will create another issue and we can discuss the reason there.

include/ginkgo/core/base/array.hpp

yhmtsai · 2023-12-07T13:16:26Z

core/test/base/array.cpp

+    EXPECT_EQ(view.get_data()[2], TypeParam{3});
+    EXPECT_EQ(view.get_size(), 3);
+    EXPECT_EQ(const_view3.get_size(), 3);
+    ASSERT_THROW(view = const_view2, gko::ValueMismatch);


if you use GKO_ENSURE_COMPATIBLE_BOUNDS, then it is accepted.

Co-authored-by: Marcel Koch <[email protected]>

- ensure equal bounds - don't make row_ptrs owning - disable conversion assignment - add in-code comment - fix tests - use this-> - add tests for incompatible reads Co-authored-by: Tobias Ribizel <[email protected]> Co-authored-by: Pratik Nayak <[email protected]> Co-authored-by: Yu-Hsiang M. Tsai <[email protected]>

sonarcloud · 2023-12-08T19:07:02Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

95.2% Coverage
0.0% Duplication

The version of Java (11.0.3) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more here

MarcelKoch added 1:ST:do-not-merge Please do not merge PR this yet. 1:ST:ready-for-review This PR is ready for review labels Nov 28, 2023

MarcelKoch self-assigned this Nov 28, 2023

ginkgo-bot added reg:testing This is related to testing. mod:core This is related to the core module. type:matrix-format This is related to the Matrix formats labels Nov 28, 2023

MarcelKoch changed the title ~~Copy into device_matrix_data into arrays~~ Copy device_matrix_data into arrays Nov 28, 2023

MarcelKoch force-pushed the correctly-copy-matrix-read branch from c0c8c28 to dc1a10f Compare November 28, 2023 09:59

MarcelKoch mentioned this pull request Nov 28, 2023

Add a CSR batched matrix format, CUDA, HIP and DPCPP kernels #1450

Merged

4 tasks

pratikvn reviewed Nov 28, 2023

View reviewed changes

core/matrix/csr.cpp Outdated Show resolved Hide resolved

core/matrix/coo.cpp Outdated Show resolved Hide resolved

upsj requested changes Nov 28, 2023

View reviewed changes

core/matrix/coo.cpp Outdated Show resolved Hide resolved

core/matrix/csr.cpp Outdated Show resolved Hide resolved

include/ginkgo/core/base/array.hpp Outdated Show resolved Hide resolved

include/ginkgo/core/base/array.hpp Outdated Show resolved Hide resolved

upsj approved these changes Nov 28, 2023

View reviewed changes

include/ginkgo/core/base/exception_helpers.hpp Outdated Show resolved Hide resolved

MarcelKoch force-pushed the correctly-copy-matrix-read branch from 49ac14f to 9d106fc Compare November 28, 2023 14:06

pratikvn approved these changes Nov 28, 2023

View reviewed changes

MarcelKoch force-pushed the correctly-copy-matrix-read branch 2 times, most recently from 6e43034 to 60c2269 Compare November 28, 2023 15:59

MarcelKoch force-pushed the correctly-copy-matrix-read branch from 30f36b5 to 8a3d846 Compare December 4, 2023 13:38

MarcelKoch requested review from upsj and pratikvn December 4, 2023 13:39

upsj requested changes Dec 4, 2023

View reviewed changes

MarcelKoch force-pushed the correctly-copy-matrix-read branch 4 times, most recently from b86c726 to 5ae2e93 Compare December 5, 2023 13:22

pratikvn requested changes Dec 5, 2023

View reviewed changes

MarcelKoch requested a review from pratikvn December 6, 2023 08:25

MarcelKoch requested a review from upsj December 6, 2023 08:25

pratikvn requested changes Dec 6, 2023

View reviewed changes

core/test/base/array.cpp Outdated Show resolved Hide resolved

MarcelKoch force-pushed the correctly-copy-matrix-read branch from ec54be8 to 93777dc Compare December 6, 2023 08:36

pratikvn approved these changes Dec 6, 2023

View reviewed changes

upsj approved these changes Dec 7, 2023

View reviewed changes

include/ginkgo/core/base/array.hpp Outdated Show resolved Hide resolved

MarcelKoch force-pushed the correctly-copy-matrix-read branch from 81d2064 to 284f979 Compare December 7, 2023 09:46

MarcelKoch added 1:ST:ready-to-merge This PR is ready to merge. and removed 1:ST:do-not-merge Please do not merge PR this yet. 1:ST:ready-for-review This PR is ready for review labels Dec 7, 2023

MarcelKoch force-pushed the correctly-copy-matrix-read branch from 284f979 to d113c9c Compare December 7, 2023 09:47

yhmtsai requested changes Dec 7, 2023

View reviewed changes

yhmtsai reviewed Dec 7, 2023

View reviewed changes

MarcelKoch force-pushed the correctly-copy-matrix-read branch 2 times, most recently from 24272c3 to a773e16 Compare December 7, 2023 11:29

yhmtsai reviewed Dec 7, 2023

View reviewed changes

MarcelKoch force-pushed the correctly-copy-matrix-read branch from a773e16 to 4f36649 Compare December 7, 2023 13:30

yhmtsai approved these changes Dec 7, 2023

View reviewed changes

MarcelKoch and others added 5 commits December 8, 2023 07:48

add copy assignment array = const_array_view

134e0c4

read copies into arrays directly

da52f55

add csr tests

94d6b1b

Format files

a18a2e3

Co-authored-by: Marcel Koch <[email protected]>

MarcelKoch force-pushed the correctly-copy-matrix-read branch from 4f36649 to c99bce3 Compare December 8, 2023 07:48

MarcelKoch merged commit a206ab6 into develop Dec 8, 2023
13 of 15 checks passed

MarcelKoch deleted the correctly-copy-matrix-read branch December 8, 2023 15:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Copy device_matrix_data into arrays #1476

Copy device_matrix_data into arrays #1476

MarcelKoch commented Nov 28, 2023 •

edited

Loading

MarcelKoch commented Nov 28, 2023

upsj left a comment

MarcelKoch commented Nov 28, 2023

MarcelKoch commented Nov 30, 2023

upsj commented Nov 30, 2023 •

edited

Loading

upsj left a comment

pratikvn left a comment

pratikvn commented Dec 6, 2023

pratikvn left a comment

upsj left a comment

yhmtsai left a comment

yhmtsai Dec 7, 2023

MarcelKoch Dec 7, 2023

yhmtsai Dec 7, 2023

upsj Dec 7, 2023

yhmtsai Dec 7, 2023

MarcelKoch commented Dec 7, 2023

yhmtsai Dec 7, 2023

MarcelKoch Dec 7, 2023

yhmtsai Dec 7, 2023

yhmtsai Dec 7, 2023

sonarcloud bot commented Dec 8, 2023

Copy device_matrix_data into arrays #1476

Copy device_matrix_data into arrays #1476

Conversation

MarcelKoch commented Nov 28, 2023 • edited Loading

MarcelKoch commented Nov 28, 2023

upsj left a comment

Choose a reason for hiding this comment

MarcelKoch commented Nov 28, 2023

MarcelKoch commented Nov 30, 2023

upsj commented Nov 30, 2023 • edited Loading

upsj left a comment

Choose a reason for hiding this comment

pratikvn left a comment

Choose a reason for hiding this comment

pratikvn commented Dec 6, 2023

pratikvn left a comment

Choose a reason for hiding this comment

upsj left a comment

Choose a reason for hiding this comment

yhmtsai 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

MarcelKoch commented Dec 7, 2023

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

sonarcloud bot commented Dec 8, 2023

MarcelKoch commented Nov 28, 2023 •

edited

Loading

upsj commented Nov 30, 2023 •

edited

Loading