-
Notifications
You must be signed in to change notification settings - Fork 97
Allow to use ValueType in template of preconditioner ILU #1828
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
f70c7b6
to
71e45e6
Compare
2b7c086
to
3bc88de
Compare
3bc88de
to
e09c3a9
Compare
6f55f04
to
fa14c74
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I only have some minor refactoring and documentation changes.
Co-authored-by: Marcel Koch <[email protected]>
Co-authored-by: Tobias Ribizel <[email protected]>
// If comp contains both factors: We only check the dimension from | ||
// the second factor. However, we still use the l_solver^H not | ||
// generate the solver on L^H to preserve the Hermitian property of | ||
// this preconditioner. LSolver(L)^H is not always LSolver^H(L^H). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MarcelKoch I checked with @upsj about this behavior. It is indeed intentional because it tries to keep the Hermitian property of IC. Thus, I revert the code changes here and the test but update documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally, transposing a preconditioner is generally cheaper than computing it, so this behavior is also preferable for many preconditioner types (except for triangular solvers, where it makes no difference, since we need to rerun the analysis in either case)
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1828 +/- ##
===========================================
- Coverage 88.77% 88.73% -0.04%
===========================================
Files 844 844
Lines 70878 70902 +24
===========================================
- Hits 62919 62916 -3
- Misses 7959 7986 +27 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This PR is the ILU part of #1811 .
As it contains LSolver and USolver,
user will need to use<ValueType, ValueType, ...>
unfortunatelyUser can use
<ValueType>
now.Also, it brings back the default l_solver in IC, which gives the LowerTrs<value_type, index_type> like original one.