Thin host matrix wrapper #140

greole · 2024-06-27T13:06:46Z

This PR adds a thin wrapper class around OpenFOAMs host LDU matrix class. The aim of this class is to simplify compute and access functions in order to create a ginkgo distributed matrix object. Ideally, this class doesn't need to be device persistent if we just make the distributed matrix class device persistent and an update function.

Additionally it introduces a communication pattern wrapper class.

src/CommunicationPattern/CommunicationPattern.H

unitTests/CommunicationPattern.C

include/OGL/CommunicationPattern.H

chihta-wang · 2024-07-04T08:13:46Z

include/OGL/CommunicationPattern.H

+    // to the corresponding target_id
+    gko::array<comm_size_type> target_sizes;
+
+    // send_idx stores the index_set ie which cells


Where is the variable index_set from?

include/OGL/CommunicationPattern.H

Co-authored-by: chihta <97195768+chihta-wang@users.noreply.github.com>

…st_wrapper

include/OGL/CommunicationPattern.H

to make it clear that there is only one owner process in the test

The unit test does not verify any functionality of CommunicationPattern. It verifies the functionality of a MPI communicator, and thus should not belong to the test suite for CommunicationPattern. To make some hardcoded unit tests more self-contained, the ascertain part for the communicator size is added accordinlgy.

1. Use vectors instead of vectors of vectors for expected results. 2. Parameterize the communicator size to avoid the repeated function calls.

… that it is not hardcoded anymore. Now it can run on an arbitrarily even number of processes.

in the increasing number of owner processes.

for the case of two owner processes

and "compute_gather_to_owner_counts_single_owner" to verify that each process can send a different number of elements to owner processes.

for the moment. Modify the unit test "compute_scatter_from_owner_counts_single_owner" so that the owner can send data to itself.

If only 1 process is used, the program will abort.

…e structures more easily identified.

…ts" for the case of two owners.

…ner_counts"

implement additional unit tests for communication pattern, see #143

unitTests/HostMatrix.C

chihta-wang

Please check the comments for the header file HostMatrix.H and for the unit test code HostMatrix.C.

We may need some unit tests for the member functions of the class HostMatrixWrapper?

chihta-wang · 2024-07-23T06:51:15Z

include/OGL/MatrixWrapper/HostMatrix.H

+ ** @param cols pointer to columns array
+ ** @param permute pointer to permuter array
+ */
+void init_local_sparsity(const label nrows, const label upper_nnz,


Why only upper non-zeros are considered? How about the lower non-zeros?

const label upper_nnz is just the number of entries in the upper triangular matrix. the number of upper_nnz==lower_nnz

Suggested change

void init_local_sparsity(const label nrows, const label upper_nnz,

void init_local_sparsity(const label nrows, const label non_diag_nnz,

i'll keep it as upper_nnz, because we already use non_diag_nnz for 2*upper_nnz

chihta-wang · 2024-07-23T06:57:06Z

include/OGL/MatrixWrapper/HostMatrix.H

+
+    const ExecutorHandler &exec_;
+
+    const DeviceIdGuardHandler device_id_guard_;


What is device_id_guard?

for more details see scoped_device_id_guard.hpp in Ginkgo

chihta-wang · 2024-07-23T07:15:35Z

include/OGL/MatrixWrapper/HostMatrix.H

+    ** ret = [(1,2,1),(2,20, 2), (3, 20, 2) ...]
+    **         i0   i1,   i...
+    */
+    [[nodiscard]] std::vector<std::tuple<label, label, label, label>>


There is a inconsistency between the comment and the function declaration regarding the return value.
In the comment, a vector of 3-element tuples is specified for the return value. However, the function declaration has a vector of 4-element tuples as the return type.

good catch.

chihta-wang · 2024-07-23T07:18:16Z

include/OGL/MatrixWrapper/HostMatrix.H

+        const lduInterfaceFieldPtrsList &interfaces) const;
+
+public:
+    // segregated matrix wrapper constructor


What is a segregated matrix?

OF uses the term segregated opposed to coupled matrix. In a coupled matrix all transport equations are stored in a single matrix. In a segregated approach each transport equation gets its own matrix.

chihta-wang · 2024-07-23T07:28:34Z

unitTests/HostMatrix.C

+    EXPECT_EQ(((HostMatrixEnvironment *)global_env)->args_->size(), 1);
+}
+
+TEST(HostMatrix, runsInParallelWithCorrectNumberOfRanks)


Since the unit tests for host matrix wrapper are hardcoded at the moment, it's better to put this test to a place where setup takes place, e.g., the body of the SetUp function

Aggree, will do that.

chihta-wang · 2024-07-23T07:41:53Z

include/OGL/MatrixWrapper/HostMatrix.H

+     ** This uses the local_sparsity_.ldu_mapping to permute the data already
+     ** on the host or device to be in row major order.
+     **/
+    void compute_local_coeffs(std::shared_ptr<const SparsityPattern> sparsity,


Is compute_local_coeffs implemented?

Currently, not. Will either added here in a separate PR or somewhere else. The reason is that it depends on the new distributed matrix wrapper class which is currently missing.

chihta-wang · 2024-07-23T07:43:18Z

include/OGL/MatrixWrapper/HostMatrix.H

+    /** Copies the interface matrix coefficients to non_local_coeffs without
+    ** changing or reinstantiating the sparsity pattern.
+    **/
+    void compute_non_local_coeffs(


Is compute_non_local_coeffs implemented?

Same as for the local_coeffs.

greole mentioned this pull request Jun 27, 2024

[Stacked] OGL Version 0.6 #134

Open

greole changed the title ~~initial commit for new thin host matrix wrapper~~ Thin host matrix wrapper Jun 27, 2024

greole commented Jul 1, 2024

View reviewed changes

src/CommunicationPattern/CommunicationPattern.H Outdated Show resolved Hide resolved

chihta-wang reviewed Jul 2, 2024

View reviewed changes

chihta-wang reviewed Jul 3, 2024

View reviewed changes

unitTests/CommunicationPattern.C Outdated Show resolved Hide resolved

unitTests/CommunicationPattern.C Show resolved Hide resolved

unitTests/CommunicationPattern.C Show resolved Hide resolved

Wip communication pattern and hostmatrix wrapper impl

19454e8

greole force-pushed the impl/host_wrapper branch from 7b6a185 to 19454e8 Compare July 3, 2024 11:26

greole added 4 commits July 3, 2024 20:29

reorder library

5838d97

reorder library

e401570

move implementation to .C file

bbed6a3

rename file

11f1612

chihta-wang reviewed Jul 4, 2024

View reviewed changes

greole and others added 9 commits July 4, 2024 11:19

prepare host matrix unittest

c871e89

add basic hostmatrix unit test

6dedfa9

add basic hostmatrix unit test

9f3882d

Apply suggestions from code review

6fe04fe

Co-authored-by: chihta <97195768+chihta-wang@users.noreply.github.com>

Merge branch 'impl/host_wrapper' of github.com:hpsim/OGL into impl/ho…

8100ab6

…st_wrapper

deactivate failing test

fa59c87

fix failing test due to call to pure virtual function

7cd98da

format files

8968895

wip read test case from disk

252c113

greole commented Jul 10, 2024

View reviewed changes

include/OGL/CommunicationPattern.H Show resolved Hide resolved

greole added 5 commits July 11, 2024 11:34

wip setup gtest

2a5af03

wip setup gtest, missing: copy of test case

f95b75e

fix link issue, add more unit tests

e9ebe7d

add non local sparsity unit tests, clean-up, format

752283f

fix send/revc offsets back for scatter pattern

5ac1a0f

greole linked an issue Jul 15, 2024 that may be closed by this pull request

The send_offsets and recv_offsets returned by the function compute_scatter_from_owner_counts #144

Closed

chihta-wang added 2 commits July 15, 2024 20:34

Rename the unit test for the function compute_owner_rank

a31291e

to make it clear that there is only one owner process in the test

chihta-wang and others added 24 commits July 15, 2024 20:36

Refactor the unit test "compute_gather_to_owner_counts_single_owner"

afa773c

1. Use vectors instead of vectors of vectors for expected results. 2. Parameterize the communicator size to avoid the repeated function calls.

Modify the unit test "compute_gather_to_owner_counts_single_owner" so…

ca5ee53

… that it is not hardcoded anymore. Now it can run on an arbitrarily even number of processes.

Reorder the unit tests for the function "compute_gather_to_owner_counts"

d812e63

in the increasing number of owner processes.

Add a new unit test for the function "compute_gather_to_owner_counts"

c547188

for the case of two owner processes

Modify the unit tests "compute_gather_to_owner_counts_all_owners"

d93a0c8

and "compute_gather_to_owner_counts_single_owner" to verify that each process can send a different number of elements to owner processes.

Add a unit test for the function "compute_scatter_from_owner_counts"

3d8c29b

owner rank 0 does not send data to itself

bbef35f

Comment out the unit test "compute_scatter_from_owner_counts_two_owners"

d6db1f4

for the moment. Modify the unit test "compute_scatter_from_owner_counts_single_owner" so that the owner can send data to itself.

Add a check for the number of CPU processes used to run the unit tests

83b7e27

If only 1 process is used, the program will abort.

Add curly braces for if statement and for loop statement to make thes…

6283a95

…e structures more easily identified.

Add a new unit test for the function "compute_scatter_from_owner_coun…

5bc25f7

…ts" for the case of two owners.

Use clang-format to set the format style for the unit test code

1b2bf82

Correct the Doxygen comment for the function "compute_scatter_from_ow…

7684c38

…ner_counts"

fix compilation on mac

6d09fb2

use test environment instead of fixture

3cda93c

Merge pull request #143 from hpsim/impl/unit_tests_communication-pattern

f420227

implement additional unit tests for communication pattern, see #143

format files

0eeadaa

format files

dcf47a3

format files

015c8d2

fix typos

0785fa4

format files

d6b4f09

format files

96ed3cd

clean up

788c203

add test case

a887d18

greole commented Jul 16, 2024

View reviewed changes

unitTests/HostMatrix.C Outdated Show resolved Hide resolved

Update unitTests/HostMatrix.C

84eac3c

chihta-wang reviewed Jul 23, 2024

View reviewed changes

greole added 2 commits July 23, 2024 10:34

move non tests to env setup

7f77698

format

9d50478

greole merged commit e6ce3e7 into stack/ogl_0.6 Jul 23, 2024
10 of 12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Thin host matrix wrapper #140

Thin host matrix wrapper #140

greole commented Jun 27, 2024 •

edited

Loading

chihta-wang Jul 4, 2024

chihta-wang left a comment •

edited

Loading

chihta-wang Jul 23, 2024

greole Jul 23, 2024

greole Jul 23, 2024

greole Jul 23, 2024

chihta-wang Jul 23, 2024

greole Jul 23, 2024

chihta-wang Jul 23, 2024

greole Jul 23, 2024

chihta-wang Jul 23, 2024

greole Jul 23, 2024

chihta-wang Jul 23, 2024

greole Jul 23, 2024

chihta-wang Jul 23, 2024

greole Jul 23, 2024

chihta-wang Jul 23, 2024

greole Jul 23, 2024

	void init_local_sparsity(const label nrows, const label upper_nnz,
	void init_local_sparsity(const label nrows, const label non_diag_nnz,


		const ExecutorHandler &exec_;

		const DeviceIdGuardHandler device_id_guard_;

Thin host matrix wrapper #140

Thin host matrix wrapper #140

Conversation

greole commented Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

chihta-wang left a comment • edited Loading

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

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

greole commented Jun 27, 2024 •

edited

Loading

chihta-wang left a comment •

edited

Loading