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 ASM as preconditioner of smoother #1210

Merged
merged 3 commits into from
Jul 31, 2024
Merged

Add ASM as preconditioner of smoother #1210

merged 3 commits into from
Jul 31, 2024

Conversation

peterrum
Copy link
Collaborator

No description provided.

@peterrum peterrum requested review from blaisb and lpsaavedra July 27, 2024 19:41
Copy link
Contributor

@blaisb blaisb left a comment

Choose a reason for hiding this comment

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

First batch of comments, will continue reviewing later

@@ -44,6 +45,209 @@

#include <deal.II/numerics/vector_tools.h>

template <typename VectorType>
class PreconditionBase
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason why you want to declare the class just in the .cc and not in the .h upstream?
Also, can you add @brief and documentation for the base class following doxygen syntax.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is there a reason why you want to declare the class just in the .cc and not in the .h upstream?

I would like to keep this local here as long as it is not needed anywhere else.

Also, can you add @brief and documentation for the base class following doxygen syntax.

Done!

source/solvers/mf_navier_stokes.cc Show resolved Hide resolved
source/solvers/mf_navier_stokes.cc Show resolved Hide resolved
Comment on lines +89 to +115
PreconditionASM()
: weighting_type(WeightingType::left)
{}
Copy link
Contributor

Choose a reason for hiding this comment

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

So right now this is still a prototype class I presume right (because the weighting type of hardcoded).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So right now this is still a prototype class I presume right (because the weighting type of hardcoded).

Yes its still a prototype. I need to make some clean up; but the version here should be enough to conduct some first experiments.

For the weighting type, one probably does not need any parameter, since the others don't really work in the NS context (I wrote the original implementation for a Poisson operator where the other variants worked better).

Copy link
Contributor

Choose a reason for hiding this comment

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

Perfect, good for me.

Copy link
Collaborator

@lpsaavedra lpsaavedra left a comment

Choose a reason for hiding this comment

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

Seems good to me! I am excited to try it out. I have only two minor comments... should we add this to the documentation? Or since it is a prototype, should we test it first? What do you think?

include/core/parameters.h Outdated Show resolved Hide resolved
source/solvers/mf_navier_stokes.cc Outdated Show resolved Hide resolved
@blaisb
Copy link
Contributor

blaisb commented Jul 31, 2024

@lpsaavedra this is very prototype-ish. Let's test it first

@blaisb blaisb merged commit 9c18482 into master Jul 31, 2024
8 checks passed
@blaisb blaisb deleted the asm branch July 31, 2024 19:58
M-Badri pushed a commit to M-Badri/lethe that referenced this pull request Sep 29, 2024

Co-authored-by: Laura Prieto Saavedra <[email protected]>
Former-commit-id: 9c18482
blaisb pushed a commit that referenced this pull request Sep 30, 2024

Co-authored-by: Laura Prieto Saavedra <[email protected]>
Former-commit-id: 9c18482
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants