Allow to use ValueType in template of preconditioner IC #1811

yhmtsai · 2025-03-19T13:28:02Z

This PR gives different way than adding a new type (#1029), which allows ValueType can be used in template LSolverType of preconditioner::Ic (similar to Ilu later).
Both PRs aims to reduce the challenges of instantiating many types for preconditioner::Ic and preconditioner::Ilu because we need to instantiate them first such that users can select one of them.

when the SolverType is LinOp,

no default solver and factorization factory in general

TODO:

reduce the instantiation type for config
Do we still accept the same configuration previously? via the LinOp interface but create the default solver when they use l_solver_type as certain type?
~~apply to ILU (will be next pr)~~ Allow to use ValueType in template of preconditioner ILU #1828

upsj

Suggestion for how we could make the changes less intrusive, and more consistent with other LinOpFactory types

include/ginkgo/core/base/composition.hpp

upsj

Looks good for the most part, a handful of questions and suggestions, and one concern about mixed precision. Maybe change the title to remove ILU?

include/ginkgo/core/preconditioner/ic.hpp

upsj · 2025-04-09T17:45:49Z

include/ginkgo/core/preconditioner/ic.hpp

-            if (!comp) {
-                GKO_NOT_SUPPORTED(comp);
+            if constexpr (std::is_same_v<value_type, void>) {
+                GKO_NOT_IMPLEMENTED;


GKO_NOT_SUPPORTED maybe?

GKO_NOT_SUPPORTED require a object, so I currently put the GKO_NOT_IMPLEMENTED

upsj · 2025-04-11T14:54:09Z

include/ginkgo/core/preconditioner/ic.hpp

-                lh_solver_ = as<lh_solver_type>(l_solver_->conj_transpose());
+            if constexpr (std::is_same_v<l_solver_type, LinOp>) {
+                // linop can not have a default factory generation
+                GKO_NOT_IMPLEMENTED;


GKO_NOT_SUPPORTED

include/ginkgo/core/preconditioner/ic.hpp

upsj · 2025-04-11T14:55:45Z

include/ginkgo/core/preconditioner/ic.hpp

+        this->set_cache_to(b);
+        if (l_solver_->apply_uses_initial_guess()) {
+            cache_.intermediate->copy_from(b);
+        }
+        l_solver_->apply(b, cache_.intermediate);
+        if (lh_solver_->apply_uses_initial_guess()) {
+            x->copy_from(cache_.intermediate);
+        }
+        lh_solver_->apply(cache_.intermediate, x);


This changes the mixed-precision behavior slightly, meaning we require more allocations, and may lose some precision. Is that intended?

I miss to delete the copy_from in the set_cache_to.
I think the current one will be less than the original one.
assume the LSolver have the same precision as IC, so cache vector is not needed to copy for precision

original one does not support mixed precision.
original: (no matter the underlying support mixed or not)
input (copy in), output (copy in)
Lsolver (always copy in set_cache_to without considering initial_guess), LHsolver no precision change
output (copy out)

now:
LSolver (copy input (in))
LHSolver (copy output (inout))
if the underlying solver support mixed precision, it will reduce the copy.

When we consider more different settings: the LSolver and LHSolver do not use the same precision
original will need to pay Lsolver(input, cache) LHSolver(cache, output) additionally because it dispatches the precision first.
but the current one: only need to pay cache conversion in LSolver and LHsolver additionally, which is less than the original one.

This is one of reason for additional stuff may need for mixed-precision (internally not just outside)

ah, do you only mean the x->copy_from(cache)?
If so, you are right, it should use the user input directly.
This part is the original behavior

It is more than what I thought because of real apply on the complex vector. I will move this out of the current pr. the apply and set_cache_to should be the same function as the original one

MarcelKoch

The change to accepting ValueType as first template parameter seems not to be done fully.

include/ginkgo/core/preconditioner/ic.hpp

MarcelKoch · 2025-04-15T12:31:54Z

include/ginkgo/core/base/utils_helper.hpp

+using factory_type = typename factory_type_impl<Type>::type;
+
+template <typename Type>
+constexpr bool is_ginkgo_linop = std::is_convertible_v<Type*, LinOp*>;


IMO std::is_base_of would be a better check.

I thought is_convertible_v more appropriate check?

At least not to me. Here we want to know if LinOp is a base class of Type, which the first suggestion does very explicitly. For the other one you have to remember that this is equivalent to LinOp being a base class.

I see your point from readability.
If we do not face the some situation (like private or ambiguous base) mentioned by std, we can go for that

include/ginkgo/core/base/utils_helper.hpp

MarcelKoch · 2025-04-15T12:41:35Z

include/ginkgo/core/preconditioner/ic.hpp

-    is_instantiation_of<Type, solver::Ir>::value ||
-    is_instantiation_of<Type, solver::Gmres>::value ||
-    is_instantiation_of<Type, preconditioner::LowerIsai>::value;
+    std::is_same_v<gko::detail::get_solver_type<SolverTypeOrValueType>, LinOp>;


I think this is more direct:

Suggested change

std::is_same_v<gko::detail::get_solver_type<SolverTypeOrValueType>, LinOp>;

!detail::is_ginkgo_linop<SolverTypeOrValueType>;

I change it a bit to only require the class not the first template, which will simplify the process for Ilu

include/ginkgo/core/preconditioner/ic.hpp

MarcelKoch

Only two minor comments left.

include/ginkgo/core/base/type_traits.hpp

include/ginkgo/core/preconditioner/ic.hpp

stale

if LSolverType is LinOp, value_type will be void and Ic will not give the default factory for l_solver and factorization

… as template Co-authored-by: Marcel Koch <marcel.koch@kit.edu> Co-authored-by: Tobias Ribizel <mail@ribizel.de>

… revert the apply due to real apply on complex vector Co-authored-by: Marcel Koch <marcel.koch@kit.edu>

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

sonarqubecloud · 2025-05-08T16:08:14Z

Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
90.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

yhmtsai added 1:ST:WIP This PR is a work in progress. Not ready for review. 1:ST:need-feedback The PR is somewhat ready but feedback on a blocking topic is required before a proper review. labels Mar 19, 2025

yhmtsai requested a review from a team March 19, 2025 13:28

yhmtsai self-assigned this Mar 19, 2025

ginkgo-bot added reg:testing This is related to testing. mod:core This is related to the core module. mod:reference This is related to the reference module. type:preconditioner This is related to the preconditioners labels Mar 19, 2025

yhmtsai force-pushed the ic_ilu_with_linop branch from a371088 to 62e7195 Compare March 19, 2025 15:22

yhmtsai added this to the Ginkgo 1.10.0 milestone Mar 26, 2025

upsj reviewed Apr 3, 2025

View reviewed changes

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

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

yhmtsai force-pushed the ic_ilu_with_linop branch from 06f44f1 to 9138755 Compare April 8, 2025 15:34

MarcelKoch self-requested a review April 9, 2025 12:43

yhmtsai requested a review from upsj April 9, 2025 17:10

upsj previously requested changes Apr 11, 2025

View reviewed changes

MarcelKoch requested changes Apr 11, 2025

View reviewed changes

include/ginkgo/core/preconditioner/ic.hpp Outdated Show resolved Hide resolved

yhmtsai changed the title ~~Allow to use LinOp as SolverType in template of preconditioner IC and ILU~~ Allow to use LinOp as SolverType in template of preconditioner IC Apr 14, 2025

yhmtsai force-pushed the ic_ilu_with_linop branch from 9138755 to 69a30a4 Compare April 15, 2025 11:47

yhmtsai requested review from MarcelKoch and upsj April 15, 2025 11:47

MarcelKoch requested changes Apr 15, 2025

View reviewed changes

yhmtsai changed the title ~~Allow to use LinOp as SolverType in template of preconditioner IC~~ Allow to use ValueType in template of preconditioner IC Apr 15, 2025

yhmtsai mentioned this pull request Apr 15, 2025

Allow to use ValueType in template of preconditioner ILU #1828

Merged

yhmtsai requested a review from MarcelKoch April 16, 2025 07:06

yhmtsai force-pushed the ic_ilu_with_linop branch from f70c7b6 to 71e45e6 Compare April 16, 2025 07:08

MarcelKoch previously requested changes Apr 16, 2025

View reviewed changes

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

include/ginkgo/core/preconditioner/ic.hpp Show resolved Hide resolved

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

yhmtsai requested a review from MarcelKoch April 17, 2025 09:14

yhmtsai force-pushed the ic_ilu_with_linop branch from 2b7c086 to 3bc88de Compare April 24, 2025 15:56

yhmtsai force-pushed the ic_ilu_with_linop branch from 3bc88de to e09c3a9 Compare April 24, 2025 16:20

yhmtsai removed the 1:ST:WIP This PR is a work in progress. Not ready for review. label Apr 28, 2025

MarcelKoch approved these changes May 5, 2025

View reviewed changes

yhmtsai 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 May 5, 2025

yhmtsai and others added 6 commits May 7, 2025 21:33

Allow to use LinOp as LSolverType in preconditioner::Ic

384c764

if LSolverType is LinOp, value_type will be void and Ic will not give the default factory for l_solver and factorization

only allow LinOp in IC during parse

70e584c

allow the first template of IC handle value type

c968b42

move the utils to utils_helper and remove unuised condition for LinOp…

d490058

… as template Co-authored-by: Marcel Koch <marcel.koch@kit.edu> Co-authored-by: Tobias Ribizel <mail@ribizel.de>

move traits due to requirement of complete type, simplify the traits,…

ee37ef8

… revert the apply due to real apply on complex vector Co-authored-by: Marcel Koch <marcel.koch@kit.edu>

remove the unused traits and add documentation

802d992

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

yhmtsai force-pushed the ic_ilu_with_linop branch from e09c3a9 to 802d992 Compare May 7, 2025 19:33

yhmtsai merged commit 6922832 into develop May 8, 2025
14 of 15 checks passed

yhmtsai deleted the ic_ilu_with_linop branch May 8, 2025 08:01

yhmtsai mentioned this pull request May 8, 2025

preconditioner Ic without the solver type #1029

Closed

2 tasks

	std::is_same_v<gko::detail::get_solver_type<SolverTypeOrValueType>, LinOp>;
	!detail::is_ginkgo_linop<SolverTypeOrValueType>;

Allow to use ValueType in template of preconditioner IC #1811

Allow to use ValueType in template of preconditioner IC #1811

Uh oh!

Conversation

yhmtsai commented Mar 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

upsj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

upsj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yhmtsai Apr 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MarcelKoch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MarcelKoch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sonarqubecloud bot commented May 8, 2025

Quality Gate passed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

yhmtsai commented Mar 19, 2025 •

edited

Loading

yhmtsai Apr 15, 2025 •

edited

Loading