Skip to content
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

Add DILU preconditioner #4899

Merged
merged 1 commit into from Oct 18, 2023
Merged

Conversation

jakobtorben
Copy link
Contributor

This PR adds the Diagonal-ILU (DILU) preconditioner to OPM-Simulators.

DILU is a simplified version of the ILU(0) where only the diagonal terms are factorised, which means that we only need to store the diagonol for applying the preconditioner. More details on the method can be found here: https://netlib.org/linalg/html_templates/node64.html#SECTION00842200000000000000

Copy link
Member

@atgeirr atgeirr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, and should be merged eventually. I have some questions below, but mostly stylistic nitpicks. I have not really delved deeply into the numerics and checked them yet, though.

Congratulations on making your first OPM PR!

opm/simulators/linalg/PreconditionerFactory_impl.hpp Outdated Show resolved Hide resolved
opm/simulators/linalg/setupPropertyTree.cpp Outdated Show resolved Hide resolved
opm/simulators/linalg/DILU.hpp Outdated Show resolved Hide resolved
opm/simulators/linalg/DILU.hpp Outdated Show resolved Hide resolved
opm/simulators/linalg/DILU.hpp Outdated Show resolved Hide resolved
opm/simulators/linalg/DILU.hpp Outdated Show resolved Hide resolved
opm/simulators/linalg/DILU.hpp Outdated Show resolved Hide resolved
for (auto i=A_.begin(); i != endi; ++i)
{
dblock rhsValue(d[i.index()]);
auto&& rhs = Impl::asVector(rhsValue);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not understand why you need the Impl::asVector() and Impl::asMatrix()? I may be missing something, is this to handle the case of using scalars as block type?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was also inspired by code from Dune. Yes it wraps a scalar as a matrix and vector to support both:

  /** \brief Wrap a scalar as a 1-1-matrix */
  template<class T,
    std::enable_if_t<IsNumber<T>::value, int> = 0>
  auto asMatrix(T& t)
  {
    return ScalarMatrixView<T>{&t};
  }

I am not sure if we need this functionality for our case. I removed it for now and everything still works. I don't see it being used anywhere else in OPM-Simulators. I can add it back in if you want.

opm/simulators/linalg/DILU.hpp Outdated Show resolved Hide resolved
opm/simulators/linalg/DILU.hpp Outdated Show resolved Hide resolved
@blattms
Copy link
Member

blattms commented Oct 6, 2023

This is a cool addition.

What I am wondering is whether this is a good candidate for our new upstream strategy. I don't think there would be any issues in getting this into dune-istl.
I will soon create an opm-branch in DUNE modules where we need it. There we can merge patches before they are officially accepted. That way we could use it OPM right away if the user uses these branches.
We sometimes prepare short publications about releases (at least for the upcoming 2.9.1 release). If authors have contributed code and help with the publications they will become authors and get citations-

@jakobtorben jakobtorben force-pushed the add_dilu_preconditioner branch 2 times, most recently from c179926 to af927cc Compare October 12, 2023 13:20
@jakobtorben
Copy link
Contributor Author

@blattms That is cool to hear.
We have gotten promising results with this preconditioner so far. Using DILU as a preconditioner for BiCgStab it as fast as ILU0 and using DILU as a smoother in CPR-AMG it is even faster than using ILU0 as a smoother. We are looking into adding a parallel multicolour-DILU implementation as well, which we expect will make it even faster.

I can add this to dune-istl as well and contribute to a release publications if needed. Just let me know when the branch is set up and maybe give me a few pointers on how to integrate it. It is based on other preconditioners in DUNE, so I expect it will be quite easy to add.

@blattms
Copy link
Member

blattms commented Oct 13, 2023

You don't need to wait until the branch is set up. These are two indepent things. Your contribution should be a merge request to the master branch. In addition I will merge it to the opm branch (Of course I still have to set that up. (How can one be so slow as me 😅 )

Due to my slowness (still need to remove a clang problem in DUNE) 2.9.1 is still not out. So there might a very little chance to get that in even there. Depends on my charms and the release manager (that is waiting for my fix).

@jakobtorben jakobtorben marked this pull request as ready for review October 17, 2023 09:21
Copy link
Member

@atgeirr atgeirr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may have some more comments, but I'll make a break now and post those I have so far.

opm/simulators/linalg/ExtraSmoothers.hpp Outdated Show resolved Hide resolved
opm/simulators/linalg/ExtraSmoothers.hpp Outdated Show resolved Hide resolved
opm/simulators/linalg/setupPropertyTree.cpp Show resolved Hide resolved
opm/simulators/linalg/PreconditionerFactory_impl.hpp Outdated Show resolved Hide resolved
opm/simulators/linalg/DILU.hpp Outdated Show resolved Hide resolved
opm/simulators/linalg/DILU.hpp Outdated Show resolved Hide resolved
@atgeirr
Copy link
Member

atgeirr commented Oct 18, 2023

jenkins build this please

@atgeirr atgeirr merged commit 27435f7 into OPM:master Oct 18, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants