Scalar parameter handling #820

MarcelKoch · 2021-07-06T08:33:07Z

This PR implements solution 1. from #816.

Todo:

adjust tests to call new overloads
(maybe) test all derived classes if the overloads work correctly

Fixes #816

MarcelKoch · 2021-07-06T08:37:15Z

Most solvers and preconditioners use friend class EnableLinOp<...>. I don't think this needs to be changed, but I might be wrong on that.

upsj

LGTM, nice job so far! You could probably also add these changes to some examples, where we had to create scalars for 0, 1 and -1 before. I guess scalar_to_dense is your way of decoupling the dense dependency from lin_op? And as a final comment, we usually try to keep two empty lines between functions and other declarations unless we are inside a class declaration or the two adjacent declarations are directly related, i.e. overloads, template specializations or similar. For more details, see https://github.com/ginkgo-project/ginkgo/wiki/Contributing-guidelines

core/CMakeLists.txt

core/base/utils.cpp

codecov · 2021-07-06T11:04:28Z

Codecov Report

Merging #820 (265e370) into develop (47ba37d) will increase coverage by 0.53%.
The diff coverage is 91.86%.

@@             Coverage Diff             @@
##           develop     #820      +/-   ##
===========================================
+ Coverage    93.71%   94.24%   +0.53%     
===========================================
  Files          408      410       +2     
  Lines        32758    32811      +53     
===========================================
+ Hits         30698    30924     +226     
+ Misses        2060     1887     -173

Impacted Files	Coverage Δ
include/ginkgo/core/base/mtx_io.hpp	`100.00% <ø> (ø)`
include/ginkgo/core/base/utils_helper.hpp	`87.17% <ø> (+2.56%)`	⬆️
include/ginkgo/core/matrix/coo.hpp	`95.00% <ø> (ø)`
include/ginkgo/core/matrix/csr.hpp	`47.72% <ø> (ø)`
include/ginkgo/core/matrix/dense.hpp	`90.69% <0.00%> (-4.43%)`	⬇️
include/ginkgo/core/matrix/ell.hpp	`100.00% <ø> (ø)`
include/ginkgo/core/matrix/fbcsr.hpp	`100.00% <ø> (ø)`
include/ginkgo/core/matrix/hybrid.hpp	`96.18% <ø> (ø)`
include/ginkgo/core/matrix/sellp.hpp	`89.74% <ø> (ø)`
core/test/base/value_typed_lin_op.cpp	`97.29% <97.29%> (ø)`
... and 26 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 47ba37d...265e370. Read the comment docs.

MarcelKoch · 2021-07-07T09:47:45Z

I'm working through the examples, as @upsj suggested, and I'm realizing that there are still some cases where this approach is not helpful. For example, in custom-logger the class ResidualLogger wants to compute something like matrix->apply(1, b, -1, x), but matrix is only available as LinOp* and thus the overload does not exist in that case. One could use a dynamic cast to EnableValuedTypedLinOp for that case, but I think that is rather clunky.

yhmtsai · 2021-07-19T08:15:49Z

include/ginkgo/core/base/lin_op.hpp

+    const ConcreteLinOp *apply(const ValueType alpha, const LinOp *b,
+                               const ValueType beta, LinOp *x) const
+    {
+        auto dense_alpha = scalar_to_dense(alpha, this->get_executor());
+        auto dense_beta = scalar_to_dense(beta, this->get_executor());
+        return this->apply(dense_alpha.get(), b, dense_beta.get(), x);
+    }


I think this part can be templated and put it in the linop class directly?
and the scalar_to_dense is free function, so there is no cycle dep

I've tried that, and it is possible. However, one problem I haven't figured out is how to handle EnableLinOp. If this is part of linop, it should be implemented again in enablelinop (or am I wrong with that?). This is more difficult, since the template needs to be instantiated explicitly somehow, and I don't know how to do this for a member function of an already templated class.

I think you does not need to instantiate the template member function

After some discussion with Tobias, I've decided to keep this in a separate mixin. Perhaps when linop gets some information about the used value type something like this could be implemented.

Is there any reason for that?
or any downside from templated member function?
If user does not use the same type as LinOp, ginkgo will handle it via mix_and_match, I think?

To make my example more concrete:

template <typename ValueType> void apply(const ValueType alpha, const LinOp *b, const ValueType beta, LinOp *x) const { auto dense_alpha = scalar_to_dense(alpha, this->get_executor()); auto dense_beta = scalar_to_dense(beta, this->get_executor()); return this->apply(dense_alpha.get(), b, dense_beta.get(), x); } ... op->apply(1, x, 0, y);

causes a linker error because scalar_to_dense<int> is not instantiated.

On the other hand, in a non-templated ValueType overload

op->apply(1, x, 0, y);

casts 1 and 0 to ValueType and at worst gives a warning about the mismatching types.

I'm trying to avoid the use of unrestricted template member functions, since users don't get a compilation error at compile time, but only a much less useful error at link time complaining about a missing scalar_to_dense<int> symbol. I believe having the concrete type in here makes for much better type-safety in the common use case, which is using concrete types instead of generic LinOp types.

the template one should be good for using.

auto dense_alpha = scalar_to_dense(int, this->get_executor); // gives Dense<float> or Dense<double> with some addition on scalar_to_dense

it will not complain the scalar_to_dense, when we add the conversion.
op->apply(1, x, 0, y); will gives implicitly conversion to ValueType, I think.

the apply will accept different input when the object is concrete type or linop
what's this target for? for all user, the developer using ginkgo or ginkgo's developer.

it will not complain the scalar_to_dense, when we add the conversion.
op->apply(1, x, 0, y); will gives implicitly conversion to ValueType, I think.

Yes, that is intended. Template type deduction requires the exact type to match, so scalar_to_dense(...) would try to use the non-existent scalar_to_dense<int> instantiation and cause a linker error. On the other hand, with an explicit non-template overload, even implicit conversions can be applied, so for a complex LinOp, you could have something like
apply(1.0, x, std::complex<double>{0.0, 1.0}, y) which works in the non-templated setting but doesn't compile in the templated setting.

what's this target for? for all user, the developer using ginkgo or ginkgo's developer.

I would imagine that this is most useful for users and maybe example development. For generic library code and things like solvers, where the repeated allocations are not negligible, I would use explicit LinOps anyways.

the apply will accept different input when the object is concrete type or linop

We have similar differences e.g. in the apply function, where LinOp::apply returns LinOp*, but EnableLinOp::apply returns a ConcreteLinOp*. More generally, only concrete types expose all functionality that the type has explicitly, which I would be totally fine with.

maybe I put thing together such that it is misleading.

// for integer Dense<float> scalar_to_dense<int>(...) // for floating Dense<float> scalar_to_dense<float>(...)

If we have this, the conversion should be okay.
current op->apply(1, x, 0, y) gives implicitly conversion to ValueType as templated one.
current pr conversion is on host but templated one may be on device.

op->apply(1.0, x, std::complex<double>{0.0, 1.0}, y) can be handled by two templated parameter for alpha and beta.

It can used when user only create the object and then use it.
If they do some combination, the type could be LinOp*

In my feeling, when I know concrete type, it is easy to use scalar_to_dense.
when I have concrete type, it is easy to know the valuetype and its executor, so I can easily to use these parameter to generate alpha/beta.
but when I have LinOp, I only have executor, so I need to use dynamic_cast through all possible types (and maybe more template params needs to know) or pick one valuetype to generate via scalar_to_dense thanks to mixed_and_match.

If we does not want scalar_to_dense<int> return Dense<float>, maybe the templated one needs to request the input always be floating point. Also, it will lead op->apply(scalar_to_dense(1, ref), ...) fail and op->apply(1, ...) work in current pr.
If we allow scalar_to_dense<int> return Dense<float>, it will be good for templated one.

It's good for example, but we start to have some LinOp like Multigrid without ValueType, so it will not always usable.
(Out of topic, I am thinking whether CG does really need ValueType because it just depends on the input matrix not the operation).
I think it is like the mixed_and_match place. It is a good tool to investigate ginkgo or develop something in ginkgo.

current pr:
pro: the conversion is host, do not contain any conversion on device.
con: needs to be concrete type or know the valuetype
templated:
pro: work for all linop
con: does not use the value type if it has one, so it might need conversion on device again.
If the linop does not implement the mixed_and_match, it will throw the link issue or runtime error?

Dense->apply return Dense and LinOp->apply return LinOP seems to be the same for me
I agree with you. only concreate type contains all functionalities but originally it's means different function name of the class to me. (maybe related to it, the naming issue compute_absolute_linop and compute_absoulte, sorry for out of topic again)

MarcelKoch · 2021-07-21T14:29:53Z

Currently, I think testing only the apply overloads of EnableValueTypedLinOp directly is sufficient. These overloads are just wrappers that call scalar_to_dense if necessary, so I think they should work in the derived classes.

upsj

LGTM for the most part! I'd like to discuss the pointer out-parameter interface though.

examples/adaptiveprecision-blockjacobi/adaptiveprecision-blockjacobi.cpp

examples/cb-gmres/cb-gmres.cpp

include/ginkgo/core/matrix/dense.hpp

include/ginkgo/core/base/mtx_io.hpp

also fixes documentation for write

not possible if: - use combination/composition - knows only LinOp*

only tests `EnableValueTypedLinOp` for correctness, no derived classes are checked

examples/adaptiveprecision-blockjacobi/adaptiveprecision-blockjacobi.cpp

examples/cb-gmres/cb-gmres.cpp

- reverts norm/dot overload with out pointer paramter Co-authored-by: Tobias Ribizel <ribizel@kit.edu>

sonarqubecloud · 2021-07-22T16:02:12Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
11 Code Smells

11.3% Coverage
4.9% Duplication

yhmtsai

nit format and need some additional documentation.
some discussion on the EnableValueTypeLinOp

yhmtsai · 2021-07-23T12:04:33Z

core/base/utils.cpp

+    std::unique_ptr<matrix::Dense<_type>> scalar_to_dense( \
+        const _type, std::shared_ptr<const Executor>)
+GKO_INSTANTIATE_FOR_EACH_VALUE_TYPE(GKO_DECLARE_SCALAR_TO_DENSE);
+


Suggested change

yhmtsai · 2021-07-23T12:04:37Z

core/base/utils.cpp

+
+
+namespace gko {
+


Suggested change

yhmtsai · 2021-07-23T12:05:34Z

core/test/base/value_typed_lin_op.cpp

+    explicit DummyValueTypedLinOp(std::shared_ptr<const gko::Executor> exec,
+                                  gko::dim<2> size = gko::dim<2>{})
+        : gko::EnableValueTypedLinOp<Self, T>(exec, size), value_()
+    {}


Suggested change

{}

{}

yhmtsai · 2021-07-23T12:06:17Z

core/test/base/value_typed_lin_op.cpp

+public:
+    explicit DummyValueTypedLinOp(std::shared_ptr<const gko::Executor> exec,
+                                  gko::dim<2> size = gko::dim<2>{})
+        : gko::EnableValueTypedLinOp<Self, T>(exec, size), value_()


maybe give zero here?

yhmtsai · 2021-07-23T12:09:38Z

core/test/base/value_typed_lin_op.cpp

+TYPED_TEST(EnableValueTypedLinOp, CanCallExtendedApplyImplLinopLinop)
+{
+    using value_type = typename TestFixture::value_type;
+    this->op->apply(lend(this->alpha), lend(this->b), lend(this->beta),


this->op can not be LinOP, right?
it needs to be the explicit type or EnableValuedTypeLinOp type

yhmtsai · 2021-07-23T12:19:59Z

examples/mixed-spmv/mixed-spmv.cpp

-              << lplp_diff_norm->at(0) / hp_x_norm->at(0) << "\n";
-    std::cout << "Lp * Hp -> Hp time(s): " << lplp_sec << std::endl;
-    std::cout << "Lp * Hp -> Hp relative error: "
-              << lphp_diff_norm->at(0) / hp_x_norm->at(0) << "\n";
+              << lplp_diff_norm->get_values()[0] / hp_x_norm->get_values()[0]
+              << "\n";


some additional delete?

yhmtsai · 2021-07-23T12:25:41Z

include/ginkgo/core/base/lin_op.hpp

+    const ConcreteLinOp *apply(const ValueType alpha, const LinOp *b,
+                               const ValueType beta, LinOp *x) const
+    {
+        auto dense_alpha = scalar_to_dense(alpha, this->get_executor());
+        auto dense_beta = scalar_to_dense(beta, this->get_executor());
+        return this->apply(dense_alpha.get(), b, dense_beta.get(), x);
+    }


In current one, you also need to know which type of linop at least which valuetype of linop.
both cases you need to know about that.
it throws error when create dense with int.
if scalar_to_dense have a specialized to handle it, it should be okay.
integer will use float to represent.

yhmtsai · 2021-07-23T12:26:19Z

include/ginkgo/core/base/lin_op.hpp

+    using EnableLinOp<ConcreteLinOp, PolymorphicBase>::EnableLinOp;
+    using EnableLinOp<ConcreteLinOp, PolymorphicBase>::apply;
+
+    const ConcreteLinOp *apply(const ValueType alpha, const LinOp *b,


need documentation

yhmtsai · 2021-07-23T12:26:56Z

include/ginkgo/core/base/mtx_io.hpp

- * Reads a matrix stored in matrix market format from an input stream.
+ * Writes a matrix into an output in the matrix market format.


good catch!

yhmtsai · 2021-07-23T12:27:42Z

include/ginkgo/core/base/utils_helper.hpp

+namespace matrix {
+template <typename ValueType>
+class Dense;
+}
+
+template <typename ValueType>
+std::unique_ptr<matrix::Dense<ValueType>> scalar_to_dense(
+    ValueType val, std::shared_ptr<const Executor> exec);
+


Suggested change

namespace matrix {

template <typename ValueType>

class Dense;

}

template <typename ValueType>

std::unique_ptr<matrix::Dense<ValueType>> scalar_to_dense(

ValueType val, std::shared_ptr<const Executor> exec);

namespace matrix {

template <typename ValueType>

class Dense;

}

template <typename ValueType>

std::unique_ptr<matrix::Dense<ValueType>> scalar_to_dense(

ValueType val, std::shared_ptr<const Executor> exec);

MarcelKoch · 2023-09-18T12:45:16Z

I'm closing this for now. ATM, there is no good way of adding the functionality.

MarcelKoch added is:enhancement An improvement of an existing feature. is:new-feature A request or implementation of a feature that does not exist yet. mod:core This is related to the core module. 1:ST:WIP This PR is a work in progress. Not ready for review. labels Jul 6, 2021

MarcelKoch self-assigned this Jul 6, 2021

upsj reviewed Jul 6, 2021

View reviewed changes

core/CMakeLists.txt Outdated Show resolved Hide resolved

core/base/utils.cpp Show resolved Hide resolved

upsj mentioned this pull request Jul 14, 2021

ResidualNorm stop can compute the needed info in gen/check #818

Merged

yhmtsai reviewed Jul 19, 2021

View reviewed changes

MarcelKoch force-pushed the scalar-parameter-handling branch from 496ce85 to 6deb8e2 Compare July 21, 2021 14:20

MarcelKoch force-pushed the scalar-parameter-handling branch from f855a3c to f044087 Compare July 21, 2021 15:26

MarcelKoch added 1:ST:ready-for-review This PR is ready for review and removed 1:ST:WIP This PR is a work in progress. Not ready for review. labels Jul 21, 2021

upsj approved these changes Jul 21, 2021

View reviewed changes

MarcelKoch added 7 commits July 22, 2021 09:36

adds mixin with apply overloads for scalar params

672e1c0

replaces EnableLinOp with EnableValueTypedLinOp

8531942

adds scalar overloads for dense

5bb7911

overloads write for single value

30e2666

also fixes documentation for write

uses new overloads in examples where possible

e5ee5cd

not possible if: - use combination/composition - knows only LinOp*

adds testing for apply with scalar input

5d8ea82

only tests `EnableValueTypedLinOp` for correctness, no derived classes are checked

fixes constness of apply overload return value

24c355b

MarcelKoch force-pushed the scalar-parameter-handling branch 2 times, most recently from 1c4c37c to e8a9a04 Compare July 22, 2021 07:39

MarcelKoch requested review from a team, upsj and yhmtsai July 22, 2021 07:42

upsj approved these changes Jul 22, 2021

View reviewed changes

examples/adaptiveprecision-blockjacobi/adaptiveprecision-blockjacobi.cpp Outdated Show resolved Hide resolved

examples/cb-gmres/cb-gmres.cpp Outdated Show resolved Hide resolved

Review update:

265e370

- reverts norm/dot overload with out pointer paramter Co-authored-by: Tobias Ribizel <ribizel@kit.edu>

MarcelKoch force-pushed the scalar-parameter-handling branch from e8a9a04 to 265e370 Compare July 22, 2021 08:27

yhmtsai requested changes Jul 23, 2021

View reviewed changes

fritzgoebel mentioned this pull request Jul 29, 2021

Add CSR scaling #848

Merged

MarcelKoch added 1:ST:need-feedback The PR is somewhat ready but feedback on a blocking topic is required before a proper review. and removed 1:ST:ready-for-review This PR is ready for review labels Mar 17, 2022

MarcelKoch added the 1:ST:WIP This PR is a work in progress. Not ready for review. label Apr 25, 2022

upsj mentioned this pull request Apr 25, 2022

Improve value semantics for LinOp types #753

Merged

27 tasks

MarcelKoch closed this Sep 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scalar parameter handling #820

Scalar parameter handling #820

MarcelKoch commented Jul 6, 2021 •

edited by upsj

Loading

MarcelKoch commented Jul 6, 2021

upsj left a comment

codecov bot commented Jul 6, 2021 •

edited

Loading

MarcelKoch commented Jul 7, 2021

yhmtsai Jul 19, 2021 •

edited

Loading

MarcelKoch Jul 20, 2021

yhmtsai Jul 21, 2021

MarcelKoch Jul 21, 2021

yhmtsai Jul 21, 2021

upsj Jul 23, 2021 •

edited

Loading

yhmtsai Jul 23, 2021

yhmtsai Jul 23, 2021

upsj Jul 23, 2021

yhmtsai Jul 23, 2021

MarcelKoch commented Jul 21, 2021

upsj left a comment

sonarqubecloud bot commented Jul 22, 2021

yhmtsai left a comment

yhmtsai Jul 23, 2021

yhmtsai Jul 23, 2021

yhmtsai Jul 23, 2021

yhmtsai Jul 23, 2021

yhmtsai Jul 23, 2021

yhmtsai Jul 23, 2021

yhmtsai Jul 23, 2021

yhmtsai Jul 23, 2021

yhmtsai Jul 23, 2021

yhmtsai Jul 23, 2021

MarcelKoch commented Sep 18, 2023

		* Reads a matrix stored in matrix market format from an input stream.
		* Writes a matrix into an output in the matrix market format.

Scalar parameter handling #820

Scalar parameter handling #820

Conversation

MarcelKoch commented Jul 6, 2021 • edited by upsj Loading

MarcelKoch commented Jul 6, 2021

upsj left a comment

Choose a reason for hiding this comment

codecov bot commented Jul 6, 2021 • edited Loading

Codecov Report

MarcelKoch commented Jul 7, 2021

yhmtsai Jul 19, 2021 • 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

upsj Jul 23, 2021 • 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

MarcelKoch commented Jul 21, 2021

upsj left a comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Jul 22, 2021

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

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 Sep 18, 2023

MarcelKoch commented Jul 6, 2021 •

edited by upsj

Loading

codecov bot commented Jul 6, 2021 •

edited

Loading

yhmtsai Jul 19, 2021 •

edited

Loading

upsj Jul 23, 2021 •

edited

Loading