Fix ilu preconditioner and extended solver #353

thoasm · 2019-09-24T15:13:48Z

This PR contains two changes (each with its own commit):

1. Make the ILU preconditioner usable as a preconditioner.

The problem is that each solver takes a LinOpFactory as a preconditioner, which the ILU was not. It was a self-written class instead of LinOpFactory, so it was possible to generate it with either one parameter (a composition), or two factors (L and U). However, that also meant that it was not possible to hand over a factory (with with_preconditioner) to any solver, making it useless as a preconditioner.
Now, it is no longer possible to take two parameters, however, it is still possible to just pass a gko::Composition<value_type> with first L and then U. Also, more importantly, it is now possible to use it as a regular preconditioner (a test also proves this now).

2. Add the possibility to give a solver factory an already generated preconditioner

Since some preconditioners are expensive to build, it can be handy to generate it once, and use it with multiple solvers at the same time. To enable this feature, each solver was now given the factory parameter generated_preconditioner. If this parameter is set, the given preconditioner is used, even if with_preconditioner (specifying a factory of a preconditioner) was used (order of importance: generated_preconditioner > preconditioner > identity matrix).
This PR allows to share the same preconditioner in many different solvers, while it also allows to use a preconditioner generated for matrix A with a solver generated for matrix B.

Also, the function set_preconditioner(std::shared_ptr<const LinOp>) was added to the abstract class Preconditionable, and implemented in all solvers. This function allows to set the preconditioner after the solver was already generated.

pratikvn

Some comments and questions

core/test/solver/ir.cpp

pratikvn · 2019-09-24T15:49:21Z

include/ginkgo/core/solver/bicgstab.hpp

@@ -134,7 +141,9 @@ class Bicgstab : public EnableLinOp<Bicgstab<ValueType>>,
          parameters_{factory->get_parameters()},
          system_matrix_{std::move(system_matrix)}
    {
-        if (parameters_.preconditioner) {
+        if (parameters_.generated_preconditioner) {


I am not sure, but I think in some cases you would like to regenerate the preconditioner and pass in a LinOpFactory. I am not sure if this strict dependency is suitable.

I would prefer a parameter, something like always_use_generated_preconditioner or something like that. I know that this increases the list of parameters, but I am not sure if there is another way.

If you want the preconditioner to be generated, you could simply not use the with_generated_preconditioner. If you provide both, one has to be chosen above another. Adding another factory parameter would not really make the situation better since you could just choose to give one or the other instead.

We could resolve this by providing a set_preconditioner() function for all solvers, however, that can lead undefined behavior when a preconditioner is set while an apply is running (also, one preconditioner is generated in the beginning without being used).
Despite that, it would allow you to use the same factory for a lot of solvers, while allowing you to change the preconditioner. The downsides are still totally manageable (it should be clear that changing the preconditioner while calculating is a bad idea).

I think the way it has been implemented currently is good enough, though a set_preconditioner method might be useful as well. I dont think there is a problem of setting the preconditioner when the apply is running, because each apply call is synchronous (?), atleast currently.

Yes, but the problem is that we might want to go asynchronous at some point, and we can't simply take this functionality back.

Just for reference: After the discussion, we agreed that I should add both the factory parameter (which is already implemented) and the set_preconditioner function.

I guess not every solver has the "set_preconditioner" function - i.e. only "preconditionable" objects.

Yes, that is how I plan to implement it.
But I will also add a set_solver for Iterative Refinement since the interface there is very much like the ones for preconditioner.

core/test/solver/bicgstab.cpp

reference/test/preconditioner/ilu.cpp

codecov · 2019-09-24T16:42:46Z

Codecov Report

Merging #353 into develop will increase coverage by 0.02%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           develop     #353      +/-   ##
===========================================
+ Coverage    97.96%   97.99%   +0.02%     
===========================================
  Files          256      256              
  Lines        18972    19216     +244     
===========================================
+ Hits         18586    18830     +244     
  Misses         386      386

Impacted Files	Coverage Δ
reference/test/solver/upper_trs.cpp	`100% <ø> (ø)`	⬆️
reference/test/solver/lower_trs.cpp	`100% <ø> (ø)`	⬆️
core/test/solver/gmres.cpp	`100% <100%> (ø)`	⬆️
core/solver/bicgstab.cpp	`100% <100%> (ø)`	⬆️
core/test/solver/lower_trs.cpp	`100% <100%> (ø)`	⬆️
include/ginkgo/core/base/lin_op.hpp	`100% <100%> (ø)`	⬆️
include/ginkgo/core/solver/gmres.hpp	`100% <100%> (ø)`	⬆️
include/ginkgo/core/preconditioner/ilu.hpp	`100% <100%> (ø)`	⬆️
core/test/solver/cg.cpp	`100% <100%> (ø)`	⬆️
include/ginkgo/core/solver/fcg.hpp	`100% <100%> (ø)`	⬆️
... and 17 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 3dd594a...4c75839. Read the comment docs.

tcojean

LGTM.

thoasm · 2019-09-27T11:32:05Z

Apparently, this has now a bit too much duplicated code for SonarCloud. I could resolve it by making the interface Preconditionable a MixIn, but I think that would break our interface. I could also add a MixIn just for this one function, but for these few lines, I don't think it is worth it.

tcojean · 2019-09-27T11:33:29Z

@thoasm yes, I was also wondering about the same thing. In general, the interface would stay the same though as you would still have get_preconditioner() in the same place (from the solvers), no?

I also don't think it's very much worth it, I thought it would also reduce a lot of tests but in fact that only removes one out of the four tests you added for every solver, which is already something I guess.

thoasm · 2019-09-27T11:39:11Z

If we would change the Preconditionable to a MixIn, we would break the interface since you could no longer do dynamic_cast<Preconditionable *> (since you would have to add the MixIn template parameter).
Currently, it would need to be a MixIn because it checks the size of the new preconditioner.

Just adding the set_preconditioner() to the interface and making an additional MixIn for the implementation would not break the interface, but that does not really reduce the amount of code, and is also probably a lot harder to read.

thoasm · 2019-09-27T11:41:14Z

@tcojean It would also not really reduce the amount of tests. For example, clone() and copy_from() both come from MixIns, but we still test them everywhere.

tcojean · 2019-09-27T12:07:48Z

Actually rather than a mixin, can we not make Preconditionable a type which solvers inherit from? That is what I was rather thinking of, in fact. We anyway already have set and get preconditioner as virtual.

thoasm · 2019-09-27T12:18:05Z

Yes, we can do that. The only problem we would have is that we could not check during the set_preconditioner if the dimensions are correct. Everything else would work fine, and it would also not break the Interface.
An Exception will be thrown during the apply of the preconditioner, which checks if the right hand side is suitable for the matrix.

I can definitively make this change. The reason why I did not is that I wanted to throw the error as soon as I detect it, so it is less searching overhead for any user that may encounter that problem.

tcojean · 2019-09-27T13:07:39Z

As I was saying, I don't think it's very useful since it removes a test per solver and remove the set_preconditioner duplication (3 lines?). Your call.

All preconditioner must have a factory of type `LinOpFactory` in order to get accepted as a preconditioner in any solver. This is now possible with ILU (a test was added to ensure that). However, as part of that adaptation, it is no longer possible to use `generate` with both L and U separately. Now, either a matrix can be passed, which will be factorized with ParIlu internally, or a composition can be passed, consisting of both L and U (in that order).

This allows us to generate a preconditioner once and use it in multiple solvers without the need to generate it again. Providing a factory is still possible as it was. However, if both a factory and a generated preconditioner is given, the generated preconditioner is used, and the factory ignored.

`set_preconditioner` can be use to change the preconditioner of a solver after it was already created & generated. An error is thrown when the newly set preconditioner has wrong dimensions. Implementation of the `set_preconditioner` and tests (including the throwing) are added for all respective solvers.

Implementation of `get_preconditioner` and `set_preconditioner` are moved from the solver to the class `Preconditionable`. Both functions are now no longer `virtual` to allow for inlining. Additionally, LowerTrs and UpperTrs no longer inherit from `Preconditionable` since they do not use the preconditioner inside the apply.

thoasm · 2019-09-27T14:59:53Z

I talked with @pratikvn, and he is in favor of putting the implementation into Preconditionable, to also avoid code duplication when adding another solver.

In my latest commit, I do exactly that. We might have to discuss if it was OK to remove the virtual attributes for set_preconditioner and get_preconditioner to allow for inlining / removing one pointer lookup.
Casting should work the same as it did previously (even dynamic_cast should work), but I am not sure if removing virtual (therefore, removing the V-Table for that interface) already counts as breaking the interface.

What do you think of these changes, @tcojean? I am fine with them, but I would also not mind to go back to the previous version (which is a trade-off between having an error thrown faster and having less code duplication, especially when creating another solver).
But the changes to LowerTrs and UpperTrs should definitively stay (removing them from being preconditionable) because they did not use the preconditioner inside the apply, and if we added it, a triangular matrix might not be triangular anymore after a preconditioner was applied.

include/ginkgo/core/solver/lower_trs.hpp

tcojean · 2019-09-27T20:32:52Z

@thoasm technically, I think you may consider removing the virtual an interface break, as you will then not have the same effects when specializing the Preconditionable interface?

thoasm · 2019-09-27T21:13:16Z

@tcojean there I am not 100% sure about, but I think it might be better to add it again, just to be sure.

- Make funktions in `Preconditionable` `virtual` - Remove `generated_preconditioner` factory parameter in LowerTrs

pratikvn

LGTM!

thoasm · 2019-09-30T14:53:29Z

@tcojean I changed the functions set_preconditioner and get_preconditioner back to being virtual. Are you OK with me merging this PR now?

tcojean · 2019-09-30T15:03:54Z

@thoasm Sure that is fine.

The Ginkgo team is proud to announce the new minor release of Ginkgo version 1.1.0. This release brings several performance improvements, adds Windows support, adds support for factorizations inside Ginkgo and a new ILU preconditioner based on ParILU algorithm, among other things. For detailed information, check the respective issue. Supported systems and requirements: + For all platforms, cmake 3.9+ + Linux and MacOS + gcc: 5.3+, 6.3+, 7.3+, 8.1+ + clang: 3.9+ + Intel compiler: 2017+ + Apple LLVM: 8.0+ + CUDA module: CUDA 9.0+ + Windows + MinGW and CygWin: gcc 5.3+, 6.3+, 7.3+, 8.1+ + Microsoft Visual Studio: VS 2017 15.7+ + CUDA module: CUDA 9.0+, Microsoft Visual Studio + OpenMP module: MinGW or CygWin. The current known issues can be found in the [known issues page](https://github.com/ginkgo-project/ginkgo/wiki/Known-Issues). Additions: + Upper and lower triangular solvers ([#327](#327), [#336](#336), [#341](#341), [#342](#342)) + New factorization support in Ginkgo, and addition of the ParILU algorithm ([#305](#305), [#315](#315), [#319](#319), [#324](#324)) + New ILU preconditioner ([#348](#348), [#353](#353)) + Windows MinGW and Cygwin support ([#347](#347)) + Windows Visual studio support ([#351](#351)) + New example showing how to use ParILU as a preconditioner ([#358](#358)) + New example on using loggers for debugging ([#360](#360)) + Add two new 9pt and 27pt stencil examples ([#300](#300), [#306](#306)) + Allow benchmarking CuSPARSE spmv formats through Ginkgo's benchmarks ([#303](#303)) + New benchmark for sparse matrix format conversions ([#312](https://github.com/ginkgo-project/ginkgo/issues/312)[#317](https://github.com/ginkgo-project/ginkgo/issues/317)) + Add conversions between CSR and Hybrid formats ([#302](#302), [#310](#310)) + Support for sorting rows in the CSR format by column idices ([#322](#322)) + Addition of a CUDA COO SpMM kernel for improved performance ([#345](#345)) + Addition of a LinOp to handle perturbations of the form (identity + scalar * basis * projector) ([#334](#334)) + New sparsity matrix representation format with Reference and OpenMP kernels ([#349](#349), [#350](#350)) Fixes: + Accelerate GMRES solver for CUDA executor ([#363](#363)) + Fix BiCGSTAB solver convergence ([#359](#359)) + Fix CGS logging by reporting the residual for every sub iteration ([#328](#328)) + Fix CSR,Dense->Sellp conversion's memory access violation ([#295](#295)) + Accelerate CSR->Ell,Hybrid conversions on CUDA ([#313](#313), [#318](#318)) + Fixed slowdown of COO SpMV on OpenMP ([#340](#340)) + Fix gcc 6.4.0 internal compiler error ([#316](#316)) + Fix compilation issue on Apple clang++ 10 ([#322](#322)) + Make Ginkgo able to compile on Intel 2017 and above ([#337](#337)) + Make the benchmarks spmv/solver use the same matrix formats ([#366](#366)) + Fix self-written isfinite function ([#348](#348)) + Fix Jacobi issues shown by cuda-memcheck Tools and ecosystem: + Multiple improvements to the CI system and tools ([#296](#296), [#311](#311), [#365](#365)) + Multiple improvements to the Ginkgo containers ([#328](#328), [#361](#361)) + Add sonarqube analysis to Ginkgo ([#304](#304), [#308](#308), [#309](#309)) + Add clang-tidy and iwyu support to Ginkgo ([#298](#298)) + Improve Ginkgo's support of xSDK M12 policy by adding the `TPL_` arguments to CMake ([#300](#300)) + Add support for the xSDK R7 policy ([#325](#325)) + Fix examples in html documentation ([#367](#367))

The Ginkgo team is proud to announce the new minor release of Ginkgo version 1.1.0. This release brings several performance improvements, adds Windows support, adds support for factorizations inside Ginkgo and a new ILU preconditioner based on ParILU algorithm, among other things. For detailed information, check the respective issue. Supported systems and requirements: + For all platforms, cmake 3.9+ + Linux and MacOS + gcc: 5.3+, 6.3+, 7.3+, 8.1+ + clang: 3.9+ + Intel compiler: 2017+ + Apple LLVM: 8.0+ + CUDA module: CUDA 9.0+ + Windows + MinGW and Cygwin: gcc 5.3+, 6.3+, 7.3+, 8.1+ + Microsoft Visual Studio: VS 2017 15.7+ + CUDA module: CUDA 9.0+, Microsoft Visual Studio + OpenMP module: MinGW or Cygwin. The current known issues can be found in the [known issues page](https://github.com/ginkgo-project/ginkgo/wiki/Known-Issues). ### Additions + Upper and lower triangular solvers ([#327](#327), [#336](#336), [#341](#341), [#342](#342)) + New factorization support in Ginkgo, and addition of the ParILU algorithm ([#305](#305), [#315](#315), [#319](#319), [#324](#324)) + New ILU preconditioner ([#348](#348), [#353](#353)) + Windows MinGW and Cygwin support ([#347](#347)) + Windows Visual Studio support ([#351](#351)) + New example showing how to use ParILU as a preconditioner ([#358](#358)) + New example on using loggers for debugging ([#360](#360)) + Add two new 9pt and 27pt stencil examples ([#300](#300), [#306](#306)) + Allow benchmarking CuSPARSE spmv formats through Ginkgo's benchmarks ([#303](#303)) + New benchmark for sparse matrix format conversions ([#312](https://github.com/ginkgo-project/ginkgo/issues/312)[#317](https://github.com/ginkgo-project/ginkgo/issues/317)) + Add conversions between CSR and Hybrid formats ([#302](#302), [#310](#310)) + Support for sorting rows in the CSR format by column idices ([#322](#322)) + Addition of a CUDA COO SpMM kernel for improved performance ([#345](#345)) + Addition of a LinOp to handle perturbations of the form (identity + scalar * basis * projector) ([#334](#334)) + New sparsity matrix representation format with Reference and OpenMP kernels ([#349](#349), [#350](#350)) ### Fixes + Accelerate GMRES solver for CUDA executor ([#363](#363)) + Fix BiCGSTAB solver convergence ([#359](#359)) + Fix CGS logging by reporting the residual for every sub iteration ([#328](#328)) + Fix CSR,Dense->Sellp conversion's memory access violation ([#295](#295)) + Accelerate CSR->Ell,Hybrid conversions on CUDA ([#313](#313), [#318](#318)) + Fixed slowdown of COO SpMV on OpenMP ([#340](#340)) + Fix gcc 6.4.0 internal compiler error ([#316](#316)) + Fix compilation issue on Apple clang++ 10 ([#322](#322)) + Make Ginkgo able to compile on Intel 2017 and above ([#337](#337)) + Make the benchmarks spmv/solver use the same matrix formats ([#366](#366)) + Fix self-written isfinite function ([#348](#348)) + Fix Jacobi issues shown by cuda-memcheck ### Tools and ecosystem improvements + Multiple improvements to the CI system and tools ([#296](#296), [#311](#311), [#365](#365)) + Multiple improvements to the Ginkgo containers ([#328](#328), [#361](#361)) + Add sonarqube analysis to Ginkgo ([#304](#304), [#308](#308), [#309](#309)) + Add clang-tidy and iwyu support to Ginkgo ([#298](#298)) + Improve Ginkgo's support of xSDK M12 policy by adding the `TPL_` arguments to CMake ([#300](#300)) + Add support for the xSDK R7 policy ([#325](#325)) + Fix examples in html documentation ([#367](#367)) Related PR: #370

thoasm added is:bug Something looks wrong. mod:core This is related to the core module. type:solver This is related to the solvers type:preconditioner This is related to the preconditioners 1:ST:ready-for-review This PR is ready for review labels Sep 24, 2019

thoasm requested review from pratikvn, yhmtsai, hartwiganzt and tcojean September 24, 2019 15:13

thoasm self-assigned this Sep 24, 2019

pratikvn reviewed Sep 24, 2019

View reviewed changes

hartwiganzt approved these changes Sep 26, 2019

View reviewed changes

tcojean mentioned this pull request Sep 27, 2019

Add a new permutation matrix format #352

Merged

2 tasks

tcojean approved these changes Sep 27, 2019

View reviewed changes

Thomas Grützmacher added 3 commits September 27, 2019 16:48

Add documentation on how to create the ILU preconditioner

7160b77

Thomas Grützmacher added 3 commits September 27, 2019 16:50

Add ParIlu and Ilu to test_install

941cb03

thoasm force-pushed the fix_ilu_precond branch from ae5c83b to c380ba8 Compare September 27, 2019 14:56

pratikvn requested changes Sep 27, 2019

View reviewed changes

include/ginkgo/core/solver/lower_trs.hpp Outdated Show resolved Hide resolved

Review update

4c75839

- Make funktions in `Preconditionable` `virtual` - Remove `generated_preconditioner` factory parameter in LowerTrs

pratikvn approved these changes Sep 30, 2019

View reviewed changes

thoasm merged commit 4776504 into develop Sep 30, 2019

thoasm deleted the fix_ilu_precond branch September 30, 2019 16:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix ilu preconditioner and extended solver #353

Fix ilu preconditioner and extended solver #353

thoasm commented Sep 24, 2019 •

edited

Loading

pratikvn left a comment

pratikvn Sep 24, 2019

thoasm Sep 24, 2019

pratikvn Sep 25, 2019

thoasm Sep 26, 2019

thoasm Sep 26, 2019

hartwiganzt Sep 26, 2019

thoasm Sep 26, 2019

codecov bot commented Sep 24, 2019 •

edited

Loading

tcojean left a comment

thoasm commented Sep 27, 2019

tcojean commented Sep 27, 2019 •

edited

Loading

thoasm commented Sep 27, 2019

thoasm commented Sep 27, 2019

tcojean commented Sep 27, 2019

thoasm commented Sep 27, 2019

tcojean commented Sep 27, 2019

thoasm commented Sep 27, 2019

tcojean commented Sep 27, 2019

thoasm commented Sep 27, 2019

pratikvn left a comment

thoasm commented Sep 30, 2019

tcojean commented Sep 30, 2019

Fix ilu preconditioner and extended solver #353

Fix ilu preconditioner and extended solver #353

Conversation

thoasm commented Sep 24, 2019 • edited Loading

1. Make the ILU preconditioner usable as a preconditioner.

2. Add the possibility to give a solver factory an already generated preconditioner

pratikvn left a comment

Choose a reason for hiding this comment

pratikvn Sep 24, 2019

Choose a reason for hiding this comment

thoasm Sep 24, 2019

Choose a reason for hiding this comment

pratikvn Sep 25, 2019

Choose a reason for hiding this comment

thoasm Sep 26, 2019

Choose a reason for hiding this comment

thoasm Sep 26, 2019

Choose a reason for hiding this comment

hartwiganzt Sep 26, 2019

Choose a reason for hiding this comment

thoasm Sep 26, 2019

Choose a reason for hiding this comment

codecov bot commented Sep 24, 2019 • edited Loading

Codecov Report

tcojean left a comment

Choose a reason for hiding this comment

thoasm commented Sep 27, 2019

tcojean commented Sep 27, 2019 • edited Loading

thoasm commented Sep 27, 2019

thoasm commented Sep 27, 2019

tcojean commented Sep 27, 2019

thoasm commented Sep 27, 2019

tcojean commented Sep 27, 2019

thoasm commented Sep 27, 2019

tcojean commented Sep 27, 2019

thoasm commented Sep 27, 2019

pratikvn left a comment

Choose a reason for hiding this comment

thoasm commented Sep 30, 2019

tcojean commented Sep 30, 2019

thoasm commented Sep 24, 2019 •

edited

Loading

codecov bot commented Sep 24, 2019 •

edited

Loading

tcojean commented Sep 27, 2019 •

edited

Loading