-
Notifications
You must be signed in to change notification settings - Fork 122
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
support matrix splitting for cpu dilu #5702
base: master
Are you sure you want to change the base?
Conversation
@@ -113,7 +134,11 @@ class MultithreadDILU : public PreconditionerWithUpdate<X, Y> | |||
if (use_multithreading) { | |||
parallelUpdate(); | |||
} else { | |||
serialUpdate(); | |||
if (split_matrix_) { | |||
serialSplitUpdate(); |
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.
The method names serial[Split]Update and parallelUpdate may be confusing since they both can be used in an MPI parallel setting, but just depend on the number of threads per process. I suggest taking the opportunity to rename them to for example singleThreaded[Split]Update and multiThreadedUpdate.
Same goes for the apply() variants.
@@ -65,16 +65,16 @@ class MultithreadDILU : public PreconditionerWithUpdate<X, Y> | |||
/*! \brief Constructor gets all parameters to operate the prec. | |||
\param A The matrix to operate on. | |||
*/ | |||
MultithreadDILU(const M& A) | |||
: A_(A) | |||
MultithreadDILU(const M& A, bool split_matrix = true) |
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.
I think you can remove the default argument. Also, here it defaults to true, whereas the default is false both in the class member, and in the factory treatment, which is confusing.
Not really sure about the intricacies of this, but one hacky way could be to create a subclass DILUSplitPreconditioner that just inherits the regular DILU and sets the splitting parameter to true, but there is probably a better way. |
Current version makes apply much faster, from 58, to 38ms in the apply, but the update gets more expensive (82->128ms).
Many cases have many applies per update so this is a reasonable option to provide.
Todo: how to make using matrix splitting a choice in the amg hierarchies?