Fixes distributed Matrix constructor with LinOp* input #1148

MarcelKoch · 2022-10-20T12:43:06Z

This PR fixes the distributed matrix creation with local and non-local templates passed in as LinOp*. Before, the LinOp* overload would only be used if the input argument has exactly the type const LinOp*. In all other cases, eg an input as LinOp* or Csr<...>*, the templated constructor would be chosen, because, after template deduction, that constructor matches better than the LinOp* one. To prevent this from happening, I've enabled the templated constructor only if the template parameter has a create<ValueType, IndexType>(exec) function.

tcojean

LGTM

core/test/mpi/distributed/CMakeLists.txt

include/ginkgo/core/distributed/matrix.hpp

pratikvn

LGTM!

Slightly related to this PR, but maybe we should to discuss our approach for the following cases, or if they even make sense.

Users providing their own matrix formats. For example, something similar to the custom-matrix-format example. Not necessarily matrix free, but a application specific format.
Future matrix formats with multiple IndexType options: MatrixType<ValueType, IndexType1,IndexType2> ? I guess we can add another constructor for that. Maybe we can generalize this ?

MarcelKoch · 2022-10-25T07:46:01Z

@pratikvn:

Users providing their own matrix formats. For example, something similar to the custom-matrix-format example. Not necessarily matrix free, but a application specific format.

That is already possible. The user can always pass in a LinOp*. If the format is templated by <ValueType, IndexType>, then they can also use the with_matrix_type helper. (This is already in the core/test/mpi/distributed/matrix.cpp test)

Future matrix formats with multiple IndexType options: MatrixType<ValueType, IndexType1,IndexType2> ? I guess we can add another constructor for that. Maybe we can generalize this ?

Again, that will immediately work with the LinOp* overloads. I don't think the templated overloads make sense there, because they will just instantiate the local/non-local matrix type with the same value and index type as the distributed matrix. If the local/non-local types have a different number of template parameters, that approach doesn't make sense.
We might still extend this approach for additional matrix template parameters, but until we have a concrete use case it might be too much of a hassle to think about.

codecov · 2022-10-25T09:25:42Z

Codecov Report

Base: 92.00% // Head: 92.00% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (74f7fa9) compared to base (c1f8bd4).
Patch coverage: 96.66% of modified lines in pull request are covered.

❗ Current head 74f7fa9 differs from pull request most recent head cf467ef. Consider uploading reports for the commit cf467ef to get more accurate results

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #1148   +/-   ##
========================================
  Coverage    92.00%   92.00%           
========================================
  Files          535      535           
  Lines        46228    46244   +16     
========================================
+ Hits         42534    42549   +15     
- Misses        3694     3695    +1

Impacted Files	Coverage Δ
core/test/mpi/distributed/matrix.cpp	`94.68% <96.29%> (+0.08%)`	⬆️
include/ginkgo/core/distributed/matrix.hpp	`85.00% <100.00%> (-1.96%)`	⬇️
reference/base/index_set_kernels.cpp	`94.11% <0.00%> (-0.09%)`	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

yhmtsai

In general LGTM.
~~For the non-const LinOp* input, does not it use std::enable_if_t<!std::is_same<const LinOp* , const MatrixType>::value> ?~~
It does not work, it needs to support the with_matrix_type, too.

yhmtsai · 2022-10-26T07:25:01Z

core/test/mpi/distributed/matrix.cpp

+        auto mat = dist_mat_type ::create(this->ref, this->comm,
+                                          expected_type_ptr.get());


Suggested change

auto mat = dist_mat_type ::create(this->ref, this->comm,

expected_type_ptr.get());

auto mat = dist_mat_type::create(this->ref, this->comm,

expected_type_ptr.get());

Is it done by clang-format?

Probably a copy-paste error

MarcelKoch · 2022-10-28T12:56:13Z

@yhmtsai could you elaborate what doesn't work? Do you mean something like create(..., const Csr*) doesn't work or is that related to something else?

MarcelKoch · 2022-10-31T21:55:28Z

format!

include/ginkgo/core/distributed/matrix.hpp

MarcelKoch · 2022-11-01T15:31:47Z

format!

questions addressed

this deactivates the templated Matrix constructor, if the template is not a matrix-type-builder, i.e. it does not have a create<value_type, index_type>(exec) function.

Co-authored-by: Marcel Koch <marcel.koch@kit.edu>

MarcelKoch · 2022-11-01T21:28:19Z

format!

Co-authored-by: Marcel Koch <marcel.koch@kit.edu>

ginkgo-bot · 2022-11-01T21:39:20Z

Error: PR already merged!

sonarqubecloud · 2022-11-02T08:14:44Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

Advertise release 1.5.0 and last changes + Add changelog, + Update third party libraries + A small fix to a CMake file See PR: #1195 The Ginkgo team is proud to announce the new Ginkgo minor release 1.5.0. This release brings many important new features such as: - MPI-based multi-node support for all matrix formats and most solvers; - full DPC++/SYCL support, - functionality and interface for GPU-resident sparse direct solvers, - an interface for wrapping solvers with scaling and reordering applied, - a new algebraic Multigrid solver/preconditioner, - improved mixed-precision support, - support for device matrix assembly, and much more. If you face an issue, please first check our [known issues page](https://github.com/ginkgo-project/ginkgo/wiki/Known-Issues) and the [open issues list](https://github.com/ginkgo-project/ginkgo/issues) and if you do not find a solution, feel free to [open a new issue](https://github.com/ginkgo-project/ginkgo/issues/new/choose) or ask a question using the [github discussions](https://github.com/ginkgo-project/ginkgo/discussions). Supported systems and requirements: + For all platforms, CMake 3.13+ + C++14 compliant compiler + Linux and macOS + GCC: 5.5+ + clang: 3.9+ + Intel compiler: 2018+ + Apple LLVM: 8.0+ + NVHPC: 22.7+ + Cray Compiler: 14.0.1+ + CUDA module: CUDA 9.2+ or NVHPC 22.7+ + HIP module: ROCm 4.0+ + DPC++ module: Intel OneAPI 2021.3 with oneMKL and oneDPL. Set the CXX compiler to `dpcpp`. + Windows + MinGW and Cygwin: GCC 5.5+ + Microsoft Visual Studio: VS 2019 + CUDA module: CUDA 9.2+, Microsoft Visual Studio + OpenMP module: MinGW or Cygwin. Algorithm and important feature additions: + Add MPI-based multi-node for all matrix formats and solvers (except GMRES and IDR). ([#676](#676), [#908](#908), [#909](#909), [#932](#932), [#951](#951), [#961](#961), [#971](#971), [#976](#976), [#985](#985), [#1007](#1007), [#1030](#1030), [#1054](#1054), [#1100](#1100), [#1148](#1148)) + Porting the remaining algorithms (preconditioners like ISAI, Jacobi, Multigrid, ParILU(T) and ParIC(T)) to DPC++/SYCL, update to SYCL 2020, and improve support and performance ([#896](#896), [#924](#924), [#928](#928), [#929](#929), [#933](#933), [#943](#943), [#960](#960), [#1057](#1057), [#1110](#1110), [#1142](#1142)) + Add a Sparse Direct interface supporting GPU-resident numerical LU factorization, symbolic Cholesky factorization, improved triangular solvers, and more ([#957](#957), [#1058](#1058), [#1072](#1072), [#1082](#1082)) + Add a ScaleReordered interface that can wrap solvers and automatically apply reorderings and scalings ([#1059](#1059)) + Add a Multigrid solver and improve the aggregation based PGM coarsening scheme ([#542](#542), [#913](#913), [#980](#980), [#982](#982), [#986](#986)) + Add infrastructure for unified, lambda-based, backend agnostic, kernels and utilize it for some simple kernels ([#833](#833), [#910](#910), [#926](#926)) + Merge different CUDA, HIP, DPC++ and OpenMP tests under a common interface ([#904](#904), [#973](#973), [#1044](#1044), [#1117](#1117)) + Add a device_matrix_data type for device-side matrix assembly ([#886](#886), [#963](#963), [#965](#965)) + Add support for mixed real/complex BLAS operations ([#864](#864)) + Add a FFT LinOp for all but DPC++/SYCL ([#701](#701)) + Add FBCSR support for NVIDIA and AMD GPUs and CPUs with OpenMP ([#775](#775)) + Add CSR scaling ([#848](#848)) + Add array::const_view and equivalent to create constant matrices from non-const data ([#890](#890)) + Add a RowGatherer LinOp supporting mixed precision to gather dense matrix rows ([#901](#901)) + Add mixed precision SparsityCsr SpMV support ([#970](#970)) + Allow creating CSR submatrix including from (possibly discontinuous) index sets ([#885](#885), [#964](#964)) + Add a scaled identity addition (M <- aI + bM) feature interface and impls for Csr and Dense ([#942](#942)) Deprecations and important changes: + Deprecate AmgxPgm in favor of the new Pgm name. ([#1149](#1149)). + Deprecate specialized residual norm classes in favor of a common `ResidualNorm` class ([#1101](#1101)) + Deprecate CamelCase non-polymorphic types in favor of snake_case versions (like array, machine_topology, uninitialized_array, index_set) ([#1031](#1031), [#1052](#1052)) + Bug fix: restrict gko::share to rvalue references (*possible interface break*) ([#1020](#1020)) + Bug fix: when using cuSPARSE's triangular solvers, specifying the factory parameter `num_rhs` is now required when solving for more than one right-hand side, otherwise an exception is thrown ([#1184](#1184)). + Drop official support for old CUDA < 9.2 ([#887](#887)) Improved performance additions: + Reuse tmp storage in reductions in solvers and add a mutable workspace to all solvers ([#1013](#1013), [#1028](#1028)) + Add HIP unsafe atomic option for AMD ([#1091](#1091)) + Prefer vendor implementations for Dense dot, conj_dot and norm2 when available ([#967](#967)). + Tuned OpenMP SellP, COO, and ELL SpMV kernels for a small number of RHS ([#809](#809)) Fixes: + Fix various compilation warnings ([#1076](#1076), [#1183](#1183), [#1189](#1189)) + Fix issues with hwloc-related tests ([#1074](#1074)) + Fix include headers for GCC 12 ([#1071](#1071)) + Fix for simple-solver-logging example ([#1066](#1066)) + Fix for potential memory leak in Logger ([#1056](#1056)) + Fix logging of mixin classes ([#1037](#1037)) + Improve value semantics for LinOp types, like moved-from state in cross-executor copy/clones ([#753](#753)) + Fix some matrix SpMV and conversion corner cases ([#905](#905), [#978](#978)) + Fix uninitialized data ([#958](#958)) + Fix CUDA version requirement for cusparseSpSM ([#953](#953)) + Fix several issues within bash-script ([#1016](#1016)) + Fixes for `NVHPC` compiler support ([#1194](#1194)) Other additions: + Simplify and properly name GMRES kernels ([#861](#861)) + Improve pkg-config support for non-CMake libraries ([#923](#923), [#1109](#1109)) + Improve gdb pretty printer ([#987](#987), [#1114](#1114)) + Add a logger highlighting inefficient allocation and copy patterns ([#1035](#1035)) + Improved and optimized test random matrix generation ([#954](#954), [#1032](#1032)) + Better CSR strategy defaults ([#969](#969)) + Add `move_from` to `PolymorphicObject` ([#997](#997)) + Remove unnecessary device_guard usage ([#956](#956)) + Improvements to the generic accessor for mixed-precision ([#727](#727)) + Add a naive lower triangular solver implementation for CUDA ([#764](#764)) + Add support for int64 indices from CUDA 11 onward with SpMV and SpGEMM ([#897](#897)) + Add a L1 norm implementation ([#900](#900)) + Add reduce_add for arrays ([#831](#831)) + Add utility to simplify Dense View creation from an existing Dense vector ([#1136](#1136)). + Add a custom transpose implementation for Fbcsr and Csr transpose for unsupported vendor types ([#1123](#1123)) + Make IDR random initilization deterministic ([#1116](#1116)) + Move the algorithm choice for triangular solvers from Csr::strategy_type to a factory parameter ([#1088](#1088)) + Update CUDA archCoresPerSM ([#1175](#1116)) + Add kernels for Csr sparsity pattern lookup ([#994](#994)) + Differentiate between structural and numerical zeros in Ell/Sellp ([#1027](#1027)) + Add a binary IO format for matrix data ([#984](#984)) + Add a tuple zip_iterator implementation ([#966](#966)) + Simplify kernel stubs and declarations ([#888](#888)) + Simplify GKO_REGISTER_OPERATION with lambdas ([#859](#859)) + Simplify copy to device in tests and examples ([#863](#863)) + More verbose output to array assertions ([#858](#858)) + Allow parallel compilation for Jacobi kernels ([#871](#871)) + Change clang-format pointer alignment to left ([#872](#872)) + Various improvements and fixes to the benchmarking framework ([#750](#750), [#759](#759), [#870](#870), [#911](#911), [#1033](#1033), [#1137](#1137)) + Various documentation improvements ([#892](#892), [#921](#921), [#950](#950), [#977](#977), [#1021](#1021), [#1068](#1068), [#1069](#1069), [#1080](#1080), [#1081](#1081), [#1108](#1108), [#1153](#1153), [#1154](#1154)) + Various CI improvements ([#868](#868), [#874](#874), [#884](#884), [#889](#889), [#899](#899), [#903](#903), [#922](#922), [#925](#925), [#930](#930), [#936](#936), [#937](#937), [#958](#958), [#882](#882), [#1011](#1011), [#1015](#1015), [#989](#989), [#1039](#1039), [#1042](#1042), [#1067](#1067), [#1073](#1073), [#1075](#1075), [#1083](#1083), [#1084](#1084), [#1085](#1085), [#1139](#1139), [#1178](#1178), [#1187](#1187))

MarcelKoch added the 1:ST:high-importance This issue/PR is of high importance and must be addressed as soon as possible. label Oct 20, 2022

MarcelKoch self-assigned this Oct 20, 2022

ginkgo-bot added mod:core This is related to the core module. reg:build This is related to the build system. reg:testing This is related to testing. labels Oct 20, 2022

tcojean requested review from tcojean and a team October 24, 2022 09:50

tcojean approved these changes Oct 24, 2022

View reviewed changes

core/test/mpi/distributed/CMakeLists.txt Show resolved Hide resolved

include/ginkgo/core/distributed/matrix.hpp Show resolved Hide resolved

pratikvn approved these changes Oct 24, 2022

View reviewed changes

yhmtsai approved these changes Oct 26, 2022

View reviewed changes

MarcelKoch force-pushed the distributed-develop branch 3 times, most recently from 0249030 to f5e88aa Compare October 27, 2022 06:53

MarcelKoch force-pushed the distributed-develop branch 2 times, most recently from 70f1120 to b59a9dd Compare October 31, 2022 12:00

Base automatically changed from distributed-develop to develop October 31, 2022 21:01

MarcelKoch force-pushed the fix-distmtx-create-from-linop branch from ea9ef2a to 76074e6 Compare October 31, 2022 21:04

MarcelKoch added the 1:ST:ready-to-merge This PR is ready to merge. label Oct 31, 2022

upsj previously requested changes Nov 1, 2022

View reviewed changes

include/ginkgo/core/distributed/matrix.hpp Outdated Show resolved Hide resolved

MarcelKoch force-pushed the fix-distmtx-create-from-linop branch from 1dd57a8 to 3c3a78d Compare November 1, 2022 15:02

MarcelKoch requested a review from upsj November 1, 2022 15:03

MarcelKoch and others added 5 commits November 1, 2022 22:22

fixes with_matrix_type helper for reference create arguments

f72fab7

uses enable_if to deactivate templated matrix constructor overloads

159d690

this deactivates the templated Matrix constructor, if the template is not a matrix-type-builder, i.e. it does not have a create<value_type, index_type>(exec) function.

adds matrix builder tests with linop* input

8ecf8bb

Format files

d3e2b8f

Co-authored-by: Marcel Koch <marcel.koch@kit.edu>

uses forward_as_tuple for with_matrix_type

f3ff338

MarcelKoch force-pushed the fix-distmtx-create-from-linop branch from cf467ef to f3ff338 Compare November 1, 2022 21:26

Format files

f847a60

Co-authored-by: Marcel Koch <marcel.koch@kit.edu>

MarcelKoch merged commit 747fc7e into develop Nov 1, 2022

MarcelKoch deleted the fix-distmtx-create-from-linop branch November 1, 2022 21:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes distributed Matrix constructor with LinOp* input #1148

Fixes distributed Matrix constructor with LinOp* input #1148

MarcelKoch commented Oct 20, 2022

tcojean left a comment

pratikvn left a comment

MarcelKoch commented Oct 25, 2022

codecov bot commented Oct 25, 2022 •

edited

Loading

yhmtsai left a comment •

edited

Loading

yhmtsai Oct 26, 2022

MarcelKoch Oct 26, 2022

MarcelKoch commented Oct 28, 2022

MarcelKoch commented Oct 31, 2022

MarcelKoch commented Nov 1, 2022

MarcelKoch commented Nov 1, 2022

ginkgo-bot commented Nov 1, 2022

sonarqubecloud bot commented Nov 2, 2022

		auto mat = dist_mat_type ::create(this->ref, this->comm,
		expected_type_ptr.get());

Fixes distributed Matrix constructor with LinOp* input #1148

Fixes distributed Matrix constructor with LinOp* input #1148

Conversation

MarcelKoch commented Oct 20, 2022

tcojean left a comment

Choose a reason for hiding this comment

pratikvn left a comment

Choose a reason for hiding this comment

MarcelKoch commented Oct 25, 2022

codecov bot commented Oct 25, 2022 • edited Loading

Codecov Report

yhmtsai left a comment • edited Loading

Choose a reason for hiding this comment

yhmtsai Oct 26, 2022

Choose a reason for hiding this comment

MarcelKoch Oct 26, 2022

Choose a reason for hiding this comment

MarcelKoch commented Oct 28, 2022

MarcelKoch commented Oct 31, 2022

MarcelKoch commented Nov 1, 2022

MarcelKoch commented Nov 1, 2022

ginkgo-bot commented Nov 1, 2022

sonarqubecloud bot commented Nov 2, 2022

codecov bot commented Oct 25, 2022 •

edited

Loading

yhmtsai left a comment •

edited

Loading