Multigrid support [drzisga/multigrid] #1097
Conversation
…artially assembled operators.
…obal dot product was used.
* Merged MultigridSolver into Multigrid class * Moved as much as possible in to the Multigrid class in order to make DiffusionMultigrid smaller * Changed interfaces such that they look like in the mockup from the 4/1/20 telecon
|
Thank you all again for the feedback. I have now added the following changes we discussed:
|
Thanks for addressing these @drzisga, the interface now looks good to me! |
| using namespace std; | ||
| using namespace mfem; | ||
|
|
||
| class DiffusionMultigrid : public Multigrid |
There was a problem hiding this comment.
I added a description here and in ex26p
| } | ||
| }; | ||
|
|
||
| int main(int argc, char *argv[]) |
There was a problem hiding this comment.
Two lines between the class and main()?
There was a problem hiding this comment.
I added a newlines here and in ex26p
linalg/eigenvalue.hpp
Outdated
| // MFEM is free software; you can redistribute it and/or modify it under the | ||
| // terms of the BSD-3 license. We welcome feedback and contributions, see file | ||
| // CONTRIBUTING.md for details. | ||
|
|
There was a problem hiding this comment.
I wonder if this will be better in solvers.?pp... what do the reviewers think?
There was a problem hiding this comment.
In this form, it "lives" in its own world. It doesn't inherit from the Solver class and does not follow any pre-existent interface. On the other hand, it is in a way a "solver".
Maybe it can be put in the terms of the Solver::Mult(...) interface and return also the eigenvector, but I'm not a big fan of that idea.
If we look at already existing eigenslover, they seem to be in the matrix files and classes. Notably, the class DenseMatrixEigensystem is in densemat.*. So if we follow that paradigm, this needs to move to operator.*
There was a problem hiding this comment.
I don't think strongly about this either way...
There was a problem hiding this comment.
There are eigensolvers also in hypre and the densemat files
There was a problem hiding this comment.
Actually, yes, I'm for moving this to operator.*, but I also don't have strong opinion.
There was a problem hiding this comment.
I moved the PowerMethod class to operator.*
|
LGTM |
|
Re-merged in |
Co-Authored-By: Tzanio Kolev <tzanio@llnl.gov>
continuous after snapping the nodes to the shere.
|
Re-merged in |
fem/bilinearform.hpp
Outdated
| /// Get the assembly level | ||
| AssemblyLevel GetAssemblyLevel() {return assembly;} | ||
| /** Returns the assembly level */ | ||
| AssemblyLevel GetAssemblyLevel() const { return assembly; } |
There was a problem hiding this comment.
There is a difference between /// ... and /** ... */ doxygen comments -- we probably should add a section about this in CONTRIBUTING.md.
In this case, we want the comment to become a "brief" documentation and since it fits on one line we can use /// ... comment. Alternatively, (or if the /// ... comment does not fit on one line) one can use /** @brief ... */ comment to create a "brief" documentation. The comment /** ... */ without @brief becomes a "long" documentation (and so does a multiline /// ... comment without @brief).
The "brief" documentation is shown next to the method when listed as part of all class methods. The "long" documentation (together with the "brief" documentation) is shown in the detailed method description which can be accessed by clicking the link "More ..." that is added by doxygen at the end of the "brief" documentation.
Doxygen has a number of such conventions that are not obvious, so it is always best to run make doc and look at the generated output, e.g. by opening doc/CodeDocumentation.html and then searching for the relevant class through the search box in the top right corner.
There was a problem hiding this comment.
Thank you very much for the information @v-dobrev. I fixed the documentation according to your comments.
This pull request adds general matrix-based and matrix-free multigrid support. Currently, only H1 spaces are supported.