Add a Tpetra-based version of Trilinos SparseMatrix and SparsityPattern. #16288

kinnewig · 2023-11-22T20:51:52Z

This is the minimal set for a SparseMatrix implementation, where my goal is to solve the Laplace equation from step-40. This is a rather large pull request and hopefully a big step towards a working Tpetra interface.

But first, why a new class instead of reworking the existing SparseMatrix class?

I know the original plan was to rework the existing TrilinosWrappers::SparseMatrix class to use Tpetra instead of Epetra. But after I started to work on that (see PR: #16052), I quickly reached a point where it was not feasible to continue with that approach.
My three main reasons to write a new class instead are:

The TrilinosWrappers::SparseMatrix is huge; it is difficult to gain an overview of what each function is supposed to do, making it very complicated for me to debug this huge class.
As the name Templated-petra (=Tpetra) suggests, Tpetra allows the use of templates and, therefore, different scalar types. However, the old class does not have any template arguments.
Another benefit of Tpetra over Epetra is that it allows the use of different Kokkos Nodes. Therefore, I added a second template parameter for the Kokkos Nodes.

Two template Arguments

As mentioned above, part of the reason for introducing a new class instead of reworking the existing one is to expose the template parameter for the scalar type, so the TpetraWrappers::SparseMatrix class can also be used with different scalars (similar to other existing SparseMatrix classes in deal.II).
The second template parameter exposes the Kokkos Node. By choosing different Kokkos Nodes for the SparseMatrix class, the code can be easily adapted to be executed on other host devices (e.g., GPUs). More work must be done to use that feature to its full potential. But we have to start somewhere.
To make the class still as similar as possible to existing SparseMatrix classes in deal.II, a reasonable default value for that template parameter is set.
I would like your opinion, especially on the (Kokkos) Node template parameter.

Included functions in the SparseMatrix class

My first intermediate goal is to use the Tpetra wrapper to solve step-40. Therefore, I focused on the functions used in step-40. Anyhow, I tried to include the set of the most important functions. If I missed something that is frequently used, please let me know!

About the Instantiation:

I only implemented a rudimentary instantiation for the TpetraWrappers::SparseMatrix class class. @jpthiele implemented an extensive instantiation for this class (see tpetra_wrappers_inst). Once this PR is accepted, he will create a follow-up PR.

@masterleinad, especially the TpetraWrappers::SparsityPattern, is based on your work from here. Is it okay for you that I have included that in this pull request?

So please take a close look at that class, and I look forward to your feedback!

masterleinad · 2023-11-25T19:44:45Z

@masterleinad, especially the TpetraWrappers::SparsityPattern, is based on your work from here. Is it okay for you that I have included that in this pull request?

Absolutely!

include/deal.II/base/template_constraints.h

bangerth

I realize that I have a bunch of pending comments, likely from quite a long time ago already. Might as well flush them.

What is the status of this patch? Do you want us to look through it in its entirety?

doc/news/changes/major/20231122SebastianKinnewig

bangerth · 2023-11-25T04:46:19Z

include/deal.II/lac/affine_constraints.templates.h

+#include <deal.II/lac/trilinos_tpetra_sparse_matrix.h>
+#include <deal.II/lac/trilinos_tpetra_vector.h>


Since you make no other changes to this file, do you actually need these #includes?

That originates from some changes that are not included in this pull request.

I removed those includes.

Edit: These changes are actually needed. Otherwise, affine_constraints.inst does not compile.

Could these lines be move to the .templates.h file then? Or ideally to the .cc file? This way they don't have to be included transitively by everyone who just wants the affine_constraints.h file.

They are already in the .templates.h file

My mistake -- I now realize that I'm looking at the .templates.h file, not the .h file.

include/deal.II/lac/trilinos_tpetra_sparse_matrix.h

kinnewig · 2023-12-22T11:44:15Z

What is the status of this patch? Do you want us to look through it in its entirety?

Thank you for your feedback so far!
This patch is still relevant, so I would be happy if you could take a closer look at this.

bangerth

Here is batch #1. Nothing of great importance -- most of what you did was just follow the old interface, which is useful because there is little to review then. I'll get to the rest next.

include/deal.II/lac/trilinos_tpetra_sparse_matrix.h

source/lac/trilinos_tpetra_sparse_matrix.cc

include/deal.II/lac/trilinos_tpetra_sparse_matrix.templates.h

bangerth · 2024-01-01T00:20:08Z

include/deal.II/lac/trilinos_tpetra_sparse_matrix.templates.h

+      using size_type = dealii::types::signed_global_dof_index;
+
+      template <typename Node>
+      using MapType =
+        Tpetra::Map<int, dealii::types::signed_global_dof_index, Node>;
+
+      template <typename Number, typename Node>
+      using MatrixType = Tpetra::
+        CrsMatrix<Number, int, dealii::types::signed_global_dof_index, Node>;
+
+      template <typename Node>
+      using GraphType =
+        Tpetra::CrsGraph<int, dealii::types::signed_global_dof_index, Node>;


MatrixType depends on the scalar type, and these are all strictly speaking copies of what is used inside the SparseMatrix class. Could we avoid making these using declarations at the global level? I'll write a patch for the existing class as well.

I am not sure what I should change here.

bangerth

I've made it down to operator/= in the sparse_matrix.templates.h file. I'll take care of the rest next year :-)

include/deal.II/lac/trilinos_tpetra_sparse_matrix.templates.h

bangerth

This is the rest of the sparse matrix stuff. This is looking good, I think I only found one real bug.

I will continue with the sparsity pattern things later.

include/deal.II/lac/trilinos_tpetra_sparse_matrix.templates.h

bangerth

Here is the sparsity pattern stuff. I think that most of it is right, but there are a few places where I'm unsure.

include/deal.II/lac/trilinos_tpetra_sparsity_pattern.h

bangerth · 2024-01-02T18:45:14Z

include/deal.II/lac/trilinos_tpetra_sparsity_pattern.h

+      Teuchos::RCP<GraphType>
+      trilinos_sparsity_pattern() const;


You can return a const Teuchos::RCP<...> & instead of the copy. This avoids creating a copy and thereby playing with the use count, avoiding a couple of nanoseconds of work :-)

You can probably do the same in other places where you return the underlying RCP.

This is against the idea of using Teuchos::RCP<...>

I don't think a few nanoseconds are worth it...

I thought of it more as a question of what pattern we want to use in general. For all non-scalar types, getter functions that are const should return a const reference. It may not matter in this specific case, but I think as a general rule it is the right thing to do and it would be nice if we did the same here :-)

We could return a Teuchos::RCP<const ...> in those places.

bangerth · 2024-01-02T18:47:58Z

include/deal.II/lac/trilinos_tpetra_sparsity_pattern.h

+      Teuchos::RCP<const MapType>
+      domain_partitioner() const;


Like here and in the following function.

source/lac/trilinos_tpetra_sparsity_pattern.cc

bangerth · 2024-01-08T23:16:37Z

Thanks for going through all of my comments. Once you think it's in good shape, squash into a small number of commits and let me know -- then we can merge!

kinnewig · 2024-01-09T19:08:51Z

I answered most of the comments. And most of them are already marked as resolved.
So please take a last look at the still open comments; otherwise, this can be merged.

Maybe one remark: As discussed in the comments, the TpetraWrappers::SparseMatrix class and the TpetraWrappers::SparsityPattern class are now templated on dealii::MemorySpace

bangerth

Let's see how this goes!

bangerth · 2024-01-09T21:54:11Z

/rebuild

bangerth · 2024-01-10T00:21:04Z

Alright, let's get this merged and see what happens!

kinnewig mentioned this pull request Nov 24, 2023

Add parallel support to TpetraWrappers::Vector. #16290

Merged

masterleinad reviewed Nov 25, 2023

View reviewed changes

include/deal.II/base/template_constraints.h Outdated Show resolved Hide resolved

bangerth reviewed Dec 21, 2023

View reviewed changes

bangerth reviewed Jan 1, 2024

View reviewed changes

bangerth reviewed Jan 2, 2024

View reviewed changes

include/deal.II/lac/trilinos_tpetra_sparse_matrix.templates.h Outdated Show resolved Hide resolved

bangerth reviewed Jan 2, 2024

View reviewed changes

Add a Tpetra-based version of Trilinos SparseMatrix and SparsityPattern.

f0eaee6

kinnewig force-pushed the tpetra_sparse_matrix branch from 2af3047 to f0eaee6 Compare January 9, 2024 15:49

Exchange NodeType with MemorySpace

0b2b5f0

kinnewig force-pushed the tpetra_sparse_matrix branch from 5d6ddf2 to 0b2b5f0 Compare January 9, 2024 18:57

bangerth approved these changes Jan 9, 2024

View reviewed changes

bangerth added the ready to test label Jan 9, 2024

bangerth merged commit b1f557f into dealii:master Jan 10, 2024
15 checks passed

bangerth mentioned this pull request Jan 10, 2024

Tpetra wrappers do not compile #16445

Closed

tamiko mentioned this pull request Jan 10, 2024

Regression tester regressed 3a2d67 #16446

Closed

This was referenced Jan 10, 2024

Avoid a warning. #16450

Merged

Fix the type of an array. #16451

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a Tpetra-based version of Trilinos SparseMatrix and SparsityPattern. #16288

Add a Tpetra-based version of Trilinos SparseMatrix and SparsityPattern. #16288

kinnewig commented Nov 22, 2023

masterleinad commented Nov 25, 2023

bangerth left a comment

bangerth Nov 25, 2023

kinnewig Dec 22, 2023

kinnewig Jan 8, 2024

bangerth Jan 8, 2024

kinnewig Jan 9, 2024

bangerth Jan 10, 2024

kinnewig commented Dec 22, 2023

bangerth left a comment

bangerth Jan 1, 2024

kinnewig Jan 9, 2024

bangerth left a comment

bangerth left a comment

bangerth left a comment

bangerth Jan 2, 2024

kinnewig Jan 8, 2024

bangerth Jan 8, 2024

kinnewig Jan 9, 2024

bangerth Jan 2, 2024

bangerth commented Jan 8, 2024

kinnewig commented Jan 9, 2024 •

edited

bangerth left a comment

bangerth commented Jan 9, 2024

bangerth commented Jan 10, 2024

		#include <deal.II/lac/trilinos_tpetra_sparse_matrix.h>
		#include <deal.II/lac/trilinos_tpetra_vector.h>

Add a Tpetra-based version of Trilinos SparseMatrix and SparsityPattern. #16288

Add a Tpetra-based version of Trilinos SparseMatrix and SparsityPattern. #16288

Conversation

kinnewig commented Nov 22, 2023

But first, why a new class instead of reworking the existing SparseMatrix class?

Two template Arguments

Included functions in the SparseMatrix class

About the Instantiation:

masterleinad commented Nov 25, 2023

bangerth 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

kinnewig commented Dec 22, 2023

bangerth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bangerth left a comment

Choose a reason for hiding this comment

bangerth left a comment

Choose a reason for hiding this comment

bangerth 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

bangerth commented Jan 8, 2024

kinnewig commented Jan 9, 2024 • edited

bangerth left a comment

Choose a reason for hiding this comment

bangerth commented Jan 9, 2024

bangerth commented Jan 10, 2024

kinnewig commented Jan 9, 2024 •

edited