Refactoring GMGPolar #65

mknaranja · 2024-09-13T12:33:05Z

Merge Request - GuideLine Checklist

Guideline to check code before resolve WIP and approval, respectively.
As many checkboxes as possible should be ticked.

Checks by code author:

Always to be checked:

There is at least one issue associated with the pull request.
New code adheres with the coding guidelines
No large data files have been added to the repository. Maximum size for files should be of the order of KB not MB. In particular avoid adding of pdf, word, or other files that cannot be change-tracked correctly by git.

If functions were changed or functionality was added:

Tests for new functionality has been added
A local test was succesful

If new functionality was added:

There is appropriate documentation of your work. (use doxygen style comments)

If new third party software is used:

Did you pay attention to its license? Please remember to add it to the wiki after successful merging.

If new mathematical methods or epidemiological terms are used:

Are new methods referenced? Did you provide further documentation?

[ ] The following questions are addressed in the documentation (if need be):

Developers (what did you do?, how can it be maintained?)
For users (how to use your work?)
For admins (how to install and configure your work?)
For documentation: Please write or update the Readme in the current working directory!

Checks by code reviewer(s):

Is the code clean of development artifacts e.g., unnecessary comments, prints, ...
The ticket goals for each associated issue are reached or problems are clearly addressed (i.e., a new issue was introduced).
There are appropriate unit tests and they pass.
The git history is clean and linearized for the merge request. All reviewers should squash commits and write a simple and meaningful commit message.
Coverage report for new code is acceptable.
No large data files have been added to the repository. Maximum size for files should be of the order of KB not MB. In particular avoid adding of pdf, word, or other files that cannot be change-tracked correctly by git.

.vscode/c_cpp_properties.json

.vscode/settings.json

CITATION.cff

LICENSE

Testing/Temporary/CTestCostData.txt

.clang-format

ds-aakash

Smoother files were the most difficult to follow. Will you will be documenting them in your report? (colouring and outside/inside sections)
A call graph can also be included in the appendix of your report

ds-aakash · 2025-01-02T10:14:00Z

include/Stencil/stencil.h

+#include <array>
+#include <initializer_list>
+
+enum class StencilType


StencilType may be a bit misleading name here. ( neighbour type ? )

Good point! I find GridPosition or StencilPosition might be clearer. Do you think one of these works better?"

e.g. enum class StencilPosition{TopLeft, Top, TopRight, ...};

ds-aakash · 2025-01-02T10:18:16Z

include/Stencil/stencil.h

+    BottomRight,
+};
+
+class Stencil


Stencil is again misleading. When I think of stencil values I would assume they store something like [ 0 1 0 ; 1 -4 1; 0 1 0 ]

While it's true that the term "stencil" is often associated with structures like convolution masks (e.g., [0, 1, 0; 1, -4, 1; 0, 1, 0]), its broader definition still fits the intended usage here.

ds-aakash · 2025-01-02T10:21:42Z

include/Stencil/stencil.h

May be add a comment that this Stencil class helps when building sparse matrices

ds-aakash · 2025-01-02T10:25:17Z

include/Level/level.h

+
+class LevelCache;
+
+class Level


Consider renaming this class and use level only for indicating multigrid level

ds-aakash · 2025-01-02T10:28:49Z

include/Level/level.h

+                                const DensityProfileCoefficients& density_profile_coefficients,
+                                const bool DirBC_Interior, const int num_omp_threads,
+                                const StencilDistributionMethod stencil_distribution_method);
+    void directSolveInPlace(Vector<double>& x) const;


Does InPlace imply you overwrite the RHS with the solution? (maybe add a comment)

ds-aakash · 2025-01-07T14:21:17Z

src/Smoother/SmootherGive/buildMatrix.cpp

+            if (row == column)                                                                                         \
+                center_matrix.main_diagonal(row) += value;                                                             \
+            else if (row == column - 1)                                                                                \
+                center_matrix.sub_diagonal(row) += value;                                                              \
+            else if (row == 0 && column == center_matrix.columns() - 1)                                                \
+                center_matrix.cyclic_corner_element() += value;                                                        \


You can use an inline function here instead of repeating this pattern (ex. update(matrix, value, row, col))

ds-aakash · 2025-01-07T14:36:15Z

src/Smoother/SmootherGive/buildMatrix.cpp

+            const int center_index = i_theta;                                                                          \
+            const int left_index   = i_theta;                                                                          \
+            const int right_index  = (i_r + 1 == numberSmootherCircles) ? 0 : i_theta;                                 \
+            const int bottom_index = i_theta_M1;                                                                       \
+            const int top_index    = i_theta_P1;                                                                       \


Add a comment about these indices (row inside their local tridiagonal matrix as you explained)

ds-aakash · 2025-01-07T16:30:04Z

src/Smoother/SmootherGive/buildMatrix.cpp

+            else {
+                auto& solverMatrix = circle_tridiagonal_solver_[circle_Asc_index];
+
+                solverMatrix = SymmetricTridiagonalSolver<double>(num_circle_nodes);
+                solverMatrix.is_cyclic(true);
+                solverMatrix.cyclic_corner_element() = 0.0;
+
+                for (int i = 0; i < num_circle_nodes; i++) {
+                    solverMatrix.main_diagonal(i) = 0.0;
+                    if (i < num_circle_nodes - 1) {
+                        solverMatrix.sub_diagonal(i) = 0.0;
+                    }
+                }
+            }
+        }


May be have a different function for zero initializations

ds-aakash · 2025-01-07T17:44:42Z

src/Smoother/SmootherGive/smootherSolver.cpp

Comments/illustrations explaining node coloring should be very helpful

ds-aakash · 2025-01-07T18:55:35Z

CMakeLists.txt

Using GLOB can cause issues when changing or adding files. CMake files are usually recursive (ex. https://gitlab.com/CLIUtils/modern-cmake/-/tree/master/examples/extended-project)

mknaranja

forget to submit them recently

mknaranja · 2024-12-22T20:47:59Z

CMakeLists.txt

+# Mumps: Sparse Matrix Solver
+set(MUMPS_PREFIX_PATH "~/spack/opt/spack/linux-ubuntu20.04-icelake/gcc-9.4.0/mumps-5.6.2-m4xrhc4mshzrxmgptzbpult3nbf6qrzk")
+include_directories(${MUMPS_PREFIX_PATH}/include)
+link_directories(${MUMPS_PREFIX_PATH}/lib)


could you make the use of MUMPS optional? such that we can run it without the corresponding option in the CI? or how did you proceed?

MUMPS is currently required for solving the system, as I have removed the custom matrix solver due to its inefficiency. Making MUMPS optional would involve reintroducing the factorization and solve methods into our COO-Matrix class. While this is technically feasible, it would require significant effort to implement and maintain.

If you'd like, I could reintroduce the custom solver to the codebase in March, but I don’t think it’s the best approach to resolve this issue.

The current issue is that the tests are failing because the MUMPS library isn't being found, right? Wouldn't it be best to install MUMPS directly in the CI environment to ensure it's available during the test runs?

CMakeLists.txt

mknaranja · 2024-12-22T20:49:10Z

CMakeLists.txt

+link_directories(${MUMPS_PREFIX_PATH}/lib)
+
+# Metis: Matrix reordering for Mumps
+set(METIS_PREFIX_PATH "~/spack/opt/spack/linux-ubuntu20.04-icelake/gcc-9.4.0/metis-5.1.0-bgoncx22w55soviybggl5ydjakvkm34v")


I know I also hardcoded MUMPS path but this should be avoided as much as possible. In particular with options activated by default. Or if it is not even an option.

mknaranja · 2024-12-22T20:51:36Z

README.md


-    git clone https://github.com/mknaranja/GMGPolar
+## Installing MUMPS using Spack


The new readme seems very nice and helpful but for CI MUMPS should be optional. Or is it included in CI too?

At the moment it is required and MUMPS would need to be added to the CI pipeline.

mknaranja · 2024-12-22T20:52:25Z

README.md

@@ -113,3 +125,67 @@ Release Notes

 	c. Non-default implicit extrapolation with smoothing of all nodes on the finest level [experimental, use with care, convergence cannot be observed with residual] (--extrapolation 2)
 6) Optimization of apply_A / build_rhs / apply_prolongation / build_Asc / apply_Asc_ortho
+
+* GMGPolar 2.0
+More points


I think it should be more or less fixed now. There were some copy-paste oversights and the formatting wasn't very consistent with the Version 1.0.0 release notes.

README.md

Co-authored-by: Martin J. Kühn <[email protected]>

Litz and others added 19 commits August 19, 2024 00:13

Removed unnecessary files

b05953f

Added scripts

6374ec3

Updated third-party folder

58d31fe

Updated Google Tests

67ba907

Improved InputFunctions - TestCases

9fd06b9

Updated Utilities

d11b14b

New GMGPolar class

6ea3568

Linear Algebra & Solvers

8a2ec45

New PolarGrid class

bef5fd4

New Level class

ec142fc

New Interpolation class

fe8dc7d

New DirectSolver class

5b9c151

New Residual class

d26435e

New Smoother class

bf07e52

Performance Improvements

7d7bbba

Small changes

152bd6d

Performance Improvements

7a1627e

FMG Interplation

670a77d

FMG Interpolation for anisotropic meshes

5536113