Add ApplyHint, Split criterion in IR

[yhmtsai]

~~IR use the mutable workspace to reuse the memory when the input #rows of rhs does not change.~~ Also, splitting the crierion in IR to eliminate one reduction computation in the end if the stopping criterion only checks the iteration count. The IR will not report the convegence reason from iteration count and residual norm check together. There are other stuff added but they migh be not used now or moved to different pr ~~- ResidualCacheable: allow give the residual workspace from outside, which is useful in multigrid when the solver can not split the iteration check and residual norm check~~ because we split criterion in IR, there is no urgent need for it. ~~- ZeroInput: the first residual of the solver is the right hand side if the input initial guess is zero. this might be different structure or another pull request~~ it is replaced by ApplyHint. use the interface not the mutable variable to pass the hint. Moreover, I can make EnableApplyHint to be feature class (not return the self pointer) or use apply If make the member function as apply, it will require the class inherit from them to use using LinOp::apply; using ApplyHint::apply to avoid the ambiguous, I think.

[upsj]

Maybe we can get #753 merged first, then we can provide this functionality for temporary vectors more universally?

[upsj]

ResidualCachable seems pretty specific to solvers, maybe we should put it into SolverBase and return a workspace ID (or invalid_index) for the residual, then we don't need a new interface?

For ZeroInput, IMO mutable state like this is really dangerous, and we should try to avoid it for anything but caching. How costly is the initial SpMV? Can we find another way to signal that the vector is zero? Ir::build().with_initial_guess(initial_guess::zero)?

[yhmtsai]

if putting into workspace structre, it will need to have the set_workspace(id, std::shared_ptr<Vector>). the object life is also extended? if taking out the workspace outside, it requires doing dummy ir apply to generate the residual vector (or making the create_or_get_op operation public.). This looks weird to me. making ir.with_initial_guess(zero); will lead to reset the initial guess in the middle of W-cycle. they do not set the initial guess to zero again. Users needs to set them separately

[upsj]

Where do you need the lifetime of the residual vector to be extended past its solver's lifetime? It sounds to me like you only want to avoid recomputing the residual by retrieving it from the smoother? Why do you need a shared_ptr at all? If the answer is "stopping criterion", that is just a design issue we need to solve on the criterion side, there should be no ownership involved. For ZeroInput, there must be a better solution interface-wise (creating two smoothers for the W cycle - the first one with zero input, the second one without?), because IMO IR's behavior should not be special-cased just for this single use-case. In general, to be feasible, the current approach would at least need to deal correctly with assignment etc.

[sonarqubecloud[bot]]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
4 Code Smells

88.2% Coverage
0.0% Duplication

[yhmtsai]

Where do you need the lifetime of the residual vector to be extended past its solver's lifetime? It sounds to me like you only want to avoid recomputing the residual by retrieving it from the smoother? Why do you need a shared_ptr at all? If the answer is "stopping criterion", that is just a design issue we need to solve on the criterion side, there should be no ownership involved.

yes, but it should be from the multigrid. it can be vector* as the current status. there are three things in this pull request and they can be considered separately and they should be possible to extend to other Solver. The multigrid solver can use any kind of solver as smoother.

For ZeroInput, there must be a better solution interface-wise (creating two smoothers for the W cycle - the first one with zero input, the second one without?), because IMO IR's behavior should not be special-cased just for this single use-case. In general, to be feasible, the current approach would at least need to deal correctly with assignment etc.

Yes, that's what I mean. but it should be controlled by user when we put it as a factory option. Actually, I prefer the matrix property to do that. The solver gets the hint from the input and uses it to reduce some unnecessary computation. The zeroinput situation depends on the input not the solver self. However, I am thinking about whether using the multigrid as a preconditioner is the same case as these situation

[upsj]

I like this approach much better, thanks for the update! I would still like to consolidate this with a potential factory parameter with_initial_guess that does the same thing, but I think we should include more people in this discussion.

[yhmtsai]

I would say with_initial_guess(zero) is changing the LinOp property like Multigrid usage (apply_use_initial_guess = false when enable). but of course, it gives more flexibility to user if they use it carefully. another quite minor benefit: we can eliminate fill(zero) by apply_hint although the benefit is quite minor, I think. I do not check this one yet.

[ginkgo-bot]

Note: This PR changes the Ginkgo ABI:

Functions changes summary: 160 Removed, 36 Changed (1656 filtered out), 185 Added functions
Variables changes summary: 0 Removed, 0 Changed, 0 Added variable

For details check the full ABI diff under Artifacts here

[sonarqubecloud[bot]]

SonarCloud Quality Gate failed.   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
86 Code Smells

89.7% Coverage
0.0% Duplication

[yhmtsai]

Rely on user setting the factory parameter.

Multigrid::build().with_presmoother(
  IR::build().with_apply_hint(zero).with_inner_solver(Jacobi::build()),
  IR::build().with_apply_hint(zero).with_inner_solver(***::build()))

with_apply_hint(zero) is required from the user. Because the smoother will be apply_initial_guess = false, Multigrid no longer to wrap a IR for these operations by the apply_initial_guess.

[yhmtsai]

https://github.com/ginkgo-project/ginkgo/commit/c440cb9622443d5281436987bd558e41d75577bd I have tried the property approach at the above link. It's not hard/complex as I thought. If the solver does not provide the apply hint, it still performs like before. apply will use the property as the hint. using apply_hint overrides the hint from the solver such that I can still use it in multigrid.

[codecov[bot]]

Codecov Report

Base: 90.89% // Head: 90.56% // Decreases project coverage by -0.33% :warning:

Coverage data is based on head (c440cb9) compared to base (6a9e459). Patch coverage: 62.33% of modified lines in pull request are covered.

:exclamation: Current head c440cb9 differs from pull request most recent head e1ea126. Consider uploading reports for the commit e1ea126 to get more accurate results

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #981      +/-   ##
===========================================
- Coverage    90.89%   90.56%   -0.34%     
===========================================
  Files          508      508              
  Lines        44293    44385      +92     
===========================================
- Hits         40262    40198      -64     
- Misses        4031     4187     +156     

Impacted Files	Coverage Δ
include/ginkgo/core/solver/solver_base.hpp	69.73% <17.24%> (-12.38%)	:arrow_down:
core/solver/ir.cpp	87.31% <87.50%> (-1.39%)	:arrow_down:
core/solver/multigrid.cpp	94.09% <100.00%> (+0.06%)	:arrow_up:
include/ginkgo/core/solver/ir.hpp	100.00% <100.00%> (ø)
test/utils/executor.hpp	25.00% <0.00%> (-75.00%)	:arrow_down:
common/unified/matrix/csr_kernels.cpp	52.30% <0.00%> (-47.70%)	:arrow_down:
common/unified/matrix/sellp_kernels.cpp	8.82% <0.00%> (-42.53%)	:arrow_down:
common/unified/matrix/ell_kernels.cpp	22.64% <0.00%> (-26.20%)	:arrow_down:
accessor/reduced_row_major.hpp	89.65% <0.00%> (-10.35%)	:arrow_down:
devices/machine_topology.cpp	77.77% <0.00%> (-4.94%)	:arrow_down:
... and 86 more

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

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

[yhmtsai]

if we would like to have the property in solver, we need to use it in apply. apply_hint(b, x, hint) is already supported. Giving the initial guess is different from the zero input. When give the zero initial guess, solver still does not know the initial guess is zero. It requires the matrix mutable property for it as I said in the beginning. I only need zero input. given is for the original purpose, and rhs is from other request.

[pratikvn]

Yes, I think for zero input, this makes sense. But I wanted to bring this general question up about having one single apply and having special cases inside controlled through the factory parameters and having different different apply functions, which I feel might be more clean ?

[yhmtsai]

@upsj I add the mixin back. Besides temporary clone and validation, I also add the logger functionality (need add friend class in the DerivedType or EnableLogger). it's not in IterativeBase, each class need to inherit EnableApplyWith... and override the impl. I do not provide the default option because it will require eight if-else dynamic case for Dense and distributed::Dense.

[sonarqubecloud[bot]]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
17 Code Smells

54.2% Coverage
0.0% Duplication

[ginkgo-bot]

Note: This PR changes the Ginkgo ABI:

Functions changes summary: 168 Removed, 24 Changed (384 filtered out), 278 Added functions
Variables changes summary: 0 Removed, 0 Changed, 0 Added variable

For details check the full ABI diff under Artifacts here

[ginkgo-bot]

Error: PR already merged!