Create matrices from const data #890

upsj · 2021-09-18T12:57:47Z

This enables the use of Array and uses it to create const LinOp pointers for matrices from existing const data.

TODO:

Add tests
~~Add example wrapping existing data (both const and non-const)~~ I think this should be addressed in a later PR.

Slaedr

Great idea to "convert" const pointers into a const matrix object. I like create_const which ensures that const Array views can be used to create only const matrices. Even though we are casting away the constness, we are at least enforcing a different but very similar contract.

Perhaps some core tests would be good.

pratikvn

Great idea! Thanks for adding this. Can we maybe prioritize this ? Would clean up some of my code.

LGTM! Only missing a few tests.

include/ginkgo/core/base/executor.hpp

include/ginkgo/core/base/array.hpp

codecov · 2021-09-19T15:12:28Z

Codecov Report

Merging #890 (bd55c34) into develop (d3deaf0) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop     #890      +/-   ##
===========================================
- Coverage    94.72%   94.71%   -0.01%     
===========================================
  Files          430      430              
  Lines        35363    35501     +138     
===========================================
+ Hits         33497    33626     +129     
- Misses        1866     1875       +9

Impacted Files	Coverage Δ
core/test/base/array.cpp	`100.00% <100.00%> (ø)`
core/test/matrix/coo.cpp	`98.79% <100.00%> (+0.14%)`	⬆️
core/test/matrix/csr.cpp	`98.88% <100.00%> (+0.13%)`	⬆️
core/test/matrix/dense.cpp	`74.76% <100.00%> (+1.23%)`	⬆️
core/test/matrix/diagonal.cpp	`100.00% <100.00%> (ø)`
core/test/matrix/ell.cpp	`98.73% <100.00%> (+0.12%)`	⬆️
core/test/matrix/fbcsr.cpp	`100.00% <100.00%> (ø)`
core/test/matrix/permutation.cpp	`98.68% <100.00%> (+0.07%)`	⬆️
core/test/matrix/sparsity_csr.cpp	`98.41% <100.00%> (+0.19%)`	⬆️
include/ginkgo/core/base/array.hpp	`94.85% <100.00%> (+0.78%)`	⬆️
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d3deaf0...bd55c34. Read the comment docs.

pratikvn

LGTM!

core/test/matrix/coo.cpp

MarcelKoch

The current code seems to assume that an array of const T might be owning its data. I don't think that is particular useful. IMO, an array of type Array<const T> should always be a view.

Other than that, I think this is a really nice addition.

include/ginkgo/core/base/array.hpp

core/test/base/array.cpp

core/test/matrix/coo.cpp

core/test/matrix/diagonal.cpp

core/test/matrix/fbcsr.cpp

Slaedr

Excellent work! I have one major concern about the array_const_cast below, and one not-so-major one about move constructors and assignments in ConstArrayView. Apart from those, this LGTM.

core/test/matrix/coo.cpp

core/test/matrix/csr.cpp

core/test/matrix/dense.cpp

Slaedr · 2021-10-06T18:02:40Z

include/ginkgo/core/base/array.hpp

+     * disable assignment altogether.
+     */
+    ConstArrayView& operator=(const ConstArrayView&) = delete;
+    ConstArrayView& operator=(ConstArrayView&&) = delete;


We could still allow move-assignment and nullify the internal pointer of the passed argument.

Here it becomes tricky: What do you do if the executors mismatch? Array would copy the data then, which is not possible for const data, so what should be the correct behavior? I wanted this type to match the behavior of Array as much as possible where it makes sense, so I would rather go for disallowing assignment altogether instead of creating some complex set of behaviors that make sense.

Maybe we just dis-allow cross-executor assignments.

That is only possible dynamically, since statically we can't determine whether executors match. Since this is only a temporary "internal" object, I think we can get away without providing full semantics here.

include/ginkgo/core/base/array.hpp

Slaedr

LGTM! I leave it to you, though I still think it would be good to have a move assignment that does the same thing as the move constructor you have now, de-prioritizing equivalence with Array (which it is not equivalent to anyway). Once we move to C++ 17, we might get rid of both the move constructor and assignment, or just keep them.

MarcelKoch

The addition of ConstArrayView seems to clear everything up that I mentioned.

upsj · 2021-10-08T13:03:06Z

rebase!

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

sonarcloud · 2021-10-09T01:33:24Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
15 Code Smells

No Coverage information
0.0% Duplication

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))

upsj added the is:idea Just a thought - if it's good, it could evolve into a proposal. label Sep 18, 2021

upsj added this to the Ginkgo 1.5.0 milestone Sep 18, 2021

upsj self-assigned this Sep 18, 2021

Slaedr reviewed Sep 18, 2021

View reviewed changes

pratikvn approved these changes Sep 19, 2021

View reviewed changes

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

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

upsj force-pushed the array_const branch from 1a05195 to 0d641af Compare October 5, 2021 08:46

upsj added the 1:ST:ready-for-review This PR is ready for review label Oct 5, 2021

upsj requested a review from a team October 5, 2021 08:46

pratikvn approved these changes Oct 5, 2021

View reviewed changes

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

MarcelKoch requested changes Oct 5, 2021

View reviewed changes

upsj force-pushed the array_const branch 2 times, most recently from b18f10a to 44f1fb1 Compare October 6, 2021 12:15

Slaedr reviewed Oct 6, 2021

View reviewed changes

Slaedr approved these changes Oct 8, 2021

View reviewed changes

MarcelKoch approved these changes Oct 8, 2021

View reviewed changes

upsj added 1:ST:ready-to-merge This PR is ready to merge. and removed 1:ST:ready-for-review This PR is ready for review labels Oct 8, 2021

upsj and others added 2 commits October 8, 2021 15:05

allow creating matrices from const data

a6baa87

test create_const with actually const data

a2d667e

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

make ConstArrayView moved-from state empty

bd55c34

upsj force-pushed the array_const branch from 7013351 to bd55c34 Compare October 8, 2021 13:05

upsj merged commit 58cac6c into develop Oct 9, 2021

upsj deleted the array_const branch October 9, 2021 08:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create matrices from const data #890

Create matrices from const data #890

upsj commented Sep 18, 2021 •

edited

Loading

Slaedr left a comment

pratikvn left a comment

codecov bot commented Sep 19, 2021 •

edited

Loading

pratikvn left a comment

MarcelKoch left a comment

Slaedr left a comment •

edited

Loading

Slaedr Oct 6, 2021

upsj Oct 6, 2021

Slaedr Oct 6, 2021

upsj Oct 8, 2021

Slaedr left a comment

MarcelKoch left a comment

upsj commented Oct 8, 2021

sonarcloud bot commented Oct 9, 2021

Create matrices from const data #890

Create matrices from const data #890

Conversation

upsj commented Sep 18, 2021 • edited Loading

Slaedr left a comment

Choose a reason for hiding this comment

pratikvn left a comment

Choose a reason for hiding this comment

codecov bot commented Sep 19, 2021 • edited Loading

Codecov Report

pratikvn left a comment

Choose a reason for hiding this comment

MarcelKoch left a comment

Choose a reason for hiding this comment

Slaedr left a comment • edited Loading

Choose a reason for hiding this comment

Slaedr Oct 6, 2021

Choose a reason for hiding this comment

upsj Oct 6, 2021

Choose a reason for hiding this comment

Slaedr Oct 6, 2021

Choose a reason for hiding this comment

upsj Oct 8, 2021

Choose a reason for hiding this comment

Slaedr left a comment

Choose a reason for hiding this comment

MarcelKoch left a comment

Choose a reason for hiding this comment

upsj commented Oct 8, 2021

sonarcloud bot commented Oct 9, 2021

upsj commented Sep 18, 2021 •

edited

Loading

codecov bot commented Sep 19, 2021 •

edited

Loading

Slaedr left a comment •

edited

Loading