scicompmod / gmgpolar Goto Github PK

In https://github.com/mknaranja/GMGPolar/blob/25cd29f5926df0fb05e6ca15a66a714694d6920d/src/smoother.cpp#L80
we want to check q*r>1 as derived in (4.4) of https://link.springer.com/article/10.1007/s10915-022-01802-1. However, with the previous line, we actually check k/h_{i-1}*r_i>1 instead of k/h_{i}*r_i>1. I think this should be corrected.

I think the correct solution is:

for (i = 1; i < nr_int - 1; i++) {
        // assume that k=k_j=theta_{j+1}-theta_{j} is constant
        double q = (theta[1] - theta[0]) / (r[i+1] - r[i]);
        if (r[i] > 1 / q)
            break;
}

Thanks to @CodingAllan for reporting. What do you think @chrissy-s96 and @Philou31 ?

Is the upper bound correct? Why was is nr_int-2 before?
-> Was this preventing that the radial smoother is empty?

In the current implementation, ntheta is always defined by nr; see https://github.com/mknaranja/GMGPolar/blob/main/src/create_grid_polar.cpp#L234

If ntheta (and anisotropy in theta) is actually used, we need also to consider: #42

a unit test, which compares the default Data as defined in main.cpp to the values of the file with name Param::filename

In GMGPolar, large code blocks for non-optimized code (optimized=0) are still available. I suppose this was only for testing that the parallel version works. Should we now remove it @Philou31 ?

error when creating fine anisotropic grid, i.e., memory error for nr_exp = fac_ani +1, fac_ani >3 and alpha_coeff = 0 or 2

As the is_bound array has been replaced, the function build_bound() can be removed from the code repository.

Originally posted by @CodingAllan in #37 (comment)

In order to speed up execution, CUDA implementations should be considered.

When compiling our software currently returns some unreachable code statements

warning: statement is unreachable [code_is_unreachable]
                      break;

which are partially due to breaks after throw commands. I am not 100% sure if the break can just be removed as the throw could be in a try/catch. Some discussion is here: https://stackoverflow.com/questions/16854308/do-i-need-a-return-after-throwing-exception-c-and-c which should be studied in detail.

In order to have a better overview, we should integrate code coverage in the CI and deploy the result to codecov.io

I think in https://github.com/mknaranja/GMGPolar/blob/ddc23c2df1f424ced9a8c905f35dc43e8308bc3e/src/prolongation.cpp#L799, we should write no bottom_left and no_top_right.

Furthermore the comment https://github.com/mknaranja/GMGPolar/blob/ddc23c2df1f424ced9a8c905f35dc43e8308bc3e/src/prolongation.cpp#L1093 is probably wrong and should be removed (as other uncessary comments of 1/2 after val=0.5).

For cases like val = k_prev * denum; // 1/2 we should write val = k_prev * denum; // isotrop: 1/2

If we allow anisotropy in theta (which basically should be possible) but was never really used due to our use cases, we have to adapt:

https://github.com/mknaranja/GMGPolar/blob/main/src/smoother.cpp#L79

Further, when we calculate "delete circles" in level::define_line_splitting() (smoother.cpp line 73), we use that the grid spacing in theta is constant, dont we ? If anisotrophy is chosen in theta I think one should update that function as well!

Originally posted by @CodingAllan in #37 (comment)

The current implementation has a lot of options (https://github.com/mknaranja/GMGPolar/blob/main/include/constants.h) refactored from the experimental Matlab code whose features where not refactored since only the most important and best working options have been transferred to C++. Unused options and all its references should be removed.

Among these options are:

• origin_NOT_coarse
• plotit (-> or better replace it by some option to export the solution in a meaningful format)
• paraview

For the options, we should think of making use

• solveit --> use it to define if compute_error() is executed?!

(To be extended)

As The code line https://github.com/mknaranja/GMGPolar/blob/ddc23c2df1f424ced9a8c905f35dc43e8308bc3e/CMakeLists.txt#L6 is considered bad practice (https://stackoverflow.com/questions/32411963/why-is-cmake-file-glob-evil) we should go over to concrete list of files. However, this can be done easily without a lot of work by just using python to create folder lists:

import os
for file in os.listdir('src/test_cases'):
    print('X' + file)

We should then probably work with add_subdirectory that contain the corresponding CMakeLists.txt.

The GMGPolar deconstructor uses levels_orig https://github.com/mknaranja/GMGPolar/blob/main/src/gmgpolar.cpp#L51 which only is set in define_coarse_nodes.cpp https://github.com/mknaranja/GMGPolar/blob/main/src/define_coarse_nodes.cpp#L52. This causes a segfault in the tests which was 'workarounded' with #44. A better strategy of default initialization or removal of levels_orig should be considered.

Mainly build_Asc and its facto.

In prolongation and restriction tests, the grid is created. For now, we will accept this but in the future, mocking should be done.

Originally posted by @mknaranja in #34 (comment)

In some occasions, we use a variable like j to be redefined multiple times inside a function while it is also used inside loops. This gives warnings like:

smoother.cpp", line 1652: warning: reference is to variable "j" (declared at line 1033) -- under old for-init scoping rules it would have been variable "j" (declared at line 1335) [hidden_by_old_for_init]
                      col = (j + 1) * ntheta_int + i;

which we should avoid by not using the same variable in two occasions redefined again and again.

Currently, some output is always given. In order to reduce the output to a minimum / zero, we should have a verbose level which suppresses all output. Furthermore, the verbose levels should be adapted consistently.

When using CUDA with NVC++ on branch `15-consider-cuda-implementation-of-essential-code-parts, we get a lot of

NVC++-S-0155-A possibly throwing copy constructor for a task firstprivate variable is not supported  (/gpfsscratch/rech/emp/utd66dd/cuda/GMGPolar/src/smoother.cpp: 2291)
NVC++-S-0155-A possibly throwing copy constructor for a task firstprivate variable is not supported  (/gpfsscratch/rech/emp/utd66dd/cuda/GMGPolar/src/smoother.cpp: 2291)
NVC++-S-0155-A possibly throwing copy constructor for a task firstprivate variable is not supported  (/gpfsscratch/rech/emp/utd66dd/cuda/GMGPolar/src/smoother.cpp: 2291)
NVC++-S-0155-A possibly throwing copy constructor for a task firstprivate variable is not supported  (/gpfsscratch/rech/emp/utd66dd/cuda/GMGPolar/src/smoother.cpp: 2291)
NVC++-S-0155-A possibly throwing copy constructor for a task firstprivate variable is not supported  (/gpfsscratch/rech/emp/utd66dd/cuda/GMGPolar/src/smoother.cpp: 2291)
NVC++-S-0155-A possibly throwing copy constructor for a task firstprivate variable is not supported  (/gpfsscratch/rech/emp/utd66dd/cuda/GMGPolar/src/smoother.cpp: 2291)

messages caused by lines like https://github.com/mknaranja/GMGPolar/blob/15-consider-cuda-implementation-of-essential-code-parts/src/smoother.cpp#L2291

The Matlab code our software was refactored from had a charming mixture of German, French, English comments. Some of these have made it to the C++ code and some functions still have German/French names like imoins. All this need to be translated to English.

When defining coarser->r and coarser->nr there is a problem with the size of the vectors. I think one can also reduce some redundancy

We already have some documentation in: https://github.com/mknaranja/GMGPolar/blob/main/include/constants.h

We should however check if this is all up to date and exhaustive.

1. In order to allow benchmarking (FLOPS_DP and CACHE) with likwid we should add a general run script.
2. The plain text results of should be scraped for important information lines and these should be exported to some data frame format.
3. The visualization of the data frame should be handled automatically with a python script.

For our scaling experiments, based on #1, we need to verify/do some more items:

• Sort and order timing outputs in GMGPolar
• Plot subcounters
• Adapt likwid options
• We need to verify that the alpha descent is aligned with the grid! (Originally posted by @mknaranja in #2 (comment)_)

We want to replace the current Makefile by a build routine that can be executed with CMake.

If the number of grid points in e.g. r is even, standard coarsening would result in two consecutive coarse circles. If we want to allow that, specialized cases had to be implemented for e.g. prolongation and restriction operators.

The code relies all the time on the fact that nr is odd, and thus nr_int (int for intervals) is even. We should exclude meshes not respecting that. We are checking nr and ntheta for the coarser grids, but we should check it also upon the grid creation; see https://github.com/mknaranja/GMGPolar/blob/main/src/define_coarse_nodes.cpp#L68

We should completely remove unused variables from our code. However, lots of warnings may come from variables that have been declared and defined for OpenMP even if OpenMP is not used. In this case we should correctly indicate when these variables are used.

when given Value 1 (--writeToFile 1) it writes the initial data
Param::prob
Param::alpha_coeff
Param::beta_coeff
Param::nr_exp
Param::ntheta_exp
Param::fac_ani
Param::mod_pk
Param::DirBC_Interior
Param::divideBy2

to file, along side with the problem size (i.e. levels, degrees of freedom nr and ntheta)

to a file with name Param::filename.

Allows to store some data for unit testing

In #8 , we unfortunately removed the option for using MUMPS. This should be corrected.

In order to allow unit testing, we want to include GoogleTest

Via the option --write_radii_angles we allow writing of the grid to the disk. However, when this is set, I get aborted and segfault in simulations:

/var/spool/slurm/d/job14283/slurm_script: line 59: break: only meaningful in a `for', `while', or `until' loop
/var/spool/slurm/d/job14283/slurm_script: line 81: 468332 Aborted                 (core dumped) ./${build_dir}/main -n $nr_exp -a $fac_ani --mod_pk $mod_pk --DirBC_Interior $DirBC_Interior --divideBy2 $divideBy2 -r $R0 --smoother $smoother --verbose 2 --debug $debug --extrapolation $extrapolation --optimized 1 --openmp $openmp --v1 $v1 --v2 $v2 -R $R --prob $prob --maxiter $maxiter --alpha_coeff $alpha_coeff --beta_coeff $beta_coeff --res_norm $res_norm --f_grid_r "radii_files_new/Rmax"$R"/aniso"$fac_ani"/divide"$divideBy2".txt" --f_grid_theta "angles_files_new/Rmax"$R"/aniso"$fac_ani"/divide"$divideBy2".txt" --rel_red_conv $rel_red_conv --write_radii_angles 1 > "outputs/job.out_"$fac_ani"_"$mod_pk"_"$prob"_"$beta_coeff"_"$extrapolation"_"$divideBy2"_"$rel_red_conv".txt" 2> "outputs/job.err_"$fac_ani"_"$mod_pk"_"$prob"_"$beta_coeff"_"$extrapolation"_"$divideBy2"_"$rel_red_conv".txt"
/var/spool/slurm/d/job14283/slurm_script: line 81: 468347 Segmentation fault      (core dumped) ./${build_dir}/main -n $nr_exp -a $fac_ani --mod_pk $mod_pk --DirBC_Interior $DirBC_Interior --divideBy2 $divideBy2 -r $R0 --smoother $smoother --verbose 2 --debug $debug --extrapolation $extrapolation --optimized 1 --openmp $openmp --v1 $v1 --v2 $v2 -R $R --prob $prob --maxiter $maxiter --alpha_coeff $alpha_coeff --beta_coeff $beta_coeff --res_norm $res_norm --f_grid_r "radii_files_new/Rmax"$R"/aniso"$fac_ani"/divide"$divideBy2".txt" --f_grid_theta "angles_files_new/Rmax"$R"/aniso"$fac_ani"/divide"$divideBy2".txt" --rel_red_conv $rel_red_conv --write_radii_angles 1 > "outputs/job.out_"$fac_ani"_"$mod_pk"_"$prob"_"$beta_coeff"_"$extrapolation"_"$divideBy2"_"$rel_red_conv".txt" 2> "outputs/job.err_"$fac_ani"_"$mod_pk"_"$prob"_"$beta_coeff"_"$extrapolation"_"$divideBy2"_"$rel_red_conv".txt"
/var/spool/slurm/d/job14283/slurm_script: line 81: 468362 Segmentation fault      (core dumped) ./${build_dir}/main -n $nr_exp -a $fac_ani --mod_pk $mod_pk --DirBC_Interior $DirBC_Interior --divideBy2 $divideBy2 -r $R0 --smoother $smoother --verbose 2 --debug $debug --extrapolation $extrapolation --optimized 1 --openmp $openmp --v1 $v1 --v2 $v2 -R $R --prob $prob --maxiter $maxiter --alpha_coeff $alpha_coeff --beta_coeff $beta_coeff --res_norm $res_norm --f_grid_r "radii_files_new/Rmax"$R"/aniso"$fac_ani"/divide"$divideBy2".txt" --f_grid_theta "angles_files_new/Rmax"$R"/aniso"$fac_ani"/divide"$divideBy2".txt" --rel_red_conv $rel_red_conv --write_radii_angles 1 > "outputs/job.out_"$fac_ani"_"$mod_pk"_"$prob"_"$beta_coeff"_"$extrapolation"_"$divideBy2"_"$rel_red_conv".txt" 2> "outputs/job.err_"$fac_ani"_"$mod_pk"_"$prob"_"$beta_coeff"_"$extrapolation"_"$divideBy2"_"$rel_red_conv".txt"
/var/spool/slurm/d/job14283/slurm_script: line 81: 468377 Segmentation fault      (core dumped) ./${build_dir}/main -n $nr_exp -a $fac_ani --mod_pk $mod_pk --DirBC_Interior $DirBC_Interior --divideBy2 $divideBy2 -r $R0 --smoother $smoother --verbose 2 --debug $debug --extrapolation $extrapolation --optimized 1 --openmp $openmp --v1 $v1 --v2 $v2 -R $R --prob $prob --maxiter $maxiter --alpha_coeff $alpha_coeff --beta_coeff $beta_coeff --res_norm $res_norm --f_grid_r "radii_files_new/Rmax"$R"/aniso"$fac_ani"/divide"$divideBy2".txt" --f_grid_theta "angles_files_new/Rmax"$R"/aniso"$fac_ani"/divide"$divideBy2".txt" --rel_red_conv $rel_red_conv --write_radii_angles 1 > "outputs/job.out_"$fac_ani"_"$mod_pk"_"$prob"_"$beta_coeff"_"$extrapolation"_"$divideBy2"_"$rel_red_conv".txt" 2> "outputs/job.err_"$fac_ani"_"$mod_pk"_"$prob"_"$beta_coeff"_"$extrapolation"_"$divideBy2"_"$rel_red_conv".txt"
/var/spool/slurm/d/job14283/slurm_script: line 81: 468392 Aborted                 (core dumped) ./${build_dir}/main -n $nr_exp -a $fac_ani --mod_pk $mod_pk --DirBC_Interior $DirBC_Interior --divideBy2 $divideBy2 -r $R0 --smoother $smoother --verbose 2 --debug $debug --extrapolation $extrapolation --optimized 1 --openmp $openmp --v1 $v1 --v2 $v2 -R $R --prob $prob --maxiter $maxiter --alpha_coeff $alpha_coeff --beta_coeff $beta_coeff --res_norm $res_norm --f_grid_r "radii_files_new/Rmax"$R"/aniso"$fac_ani"/divide"$divideBy2".txt" --f_grid_theta "angles_files_new/Rmax"$R"/aniso"$fac_ani"/divide"$divideBy2".txt" --rel_red_conv $rel_red_conv --write_radii_angles 1 > "outputs/job.out_"$fac_ani"_"$mod_pk"_"$prob"_"$beta_coeff"_"$extrapolation"_"$divideBy2"_"$rel_red_conv".txt" 2> "outputs/job.err_"$fac_ani"_"$mod_pk"_"$prob"_"$beta_coeff"_"$extrapolation"_"$divideBy2"_"$rel_red_conv".txt"
/var/spool/slurm/d/job14283/slurm_script: line 81: 468407 Aborted                 (core dumped) ./${build_dir}/main -n $nr_exp -a $fac_ani --mod_pk $mod_pk --DirBC_Interior $DirBC_Interior --divideBy2 $divideBy2 -r $R0 --smoother $smoother --verbose 2 --debug $debug --extrapolation $extrapolation --optimized 1 --openmp $openmp --v1 $v1 --v2 $v2 -R $R --prob $prob --maxiter $maxiter --alpha_coeff $alpha_coeff --beta_coeff $beta_coeff --res_norm $res_norm --f_grid_r "radii_files_new/Rmax"$R"/aniso"$fac_ani"/divide"$divideBy2".txt" --f_grid_theta "angles_files_new/Rmax"$R"/aniso"$fac_ani"/divide"$divideBy2".txt" --rel_red_conv $rel_red_conv --write_radii_angles 1 > "outputs/job.out_"$fac_ani"_"$mod_pk"_"$prob"_"$beta_coeff"_"$extrapolation"_"$divideBy2"_"$rel_red_conv".txt" 2> "outputs/job.err_"$fac_ani"_"$mod_pk"_"$prob"_"$beta_coeff"_"$extrapolation"_"$divideBy2"_"$rel_red_conv".txt"

Only when having written the data, removing the option, and restarting without writing (e.g. with reading) I can do the simulation.

In https://github.com/mknaranja/GMGPolar/blob/main/src/prolongation.cpp#L100-L114 we consider the prolongation of the second circle (row = j * ntheta_int + i;). While we have coarse nodes on the first circle, there are only fine nodes on the second circle. If we look at the interpolation operator description we made, we would access the node before and the node after in theta direction. However, this is not done. The same applies to the other prolongation and restriction operators (also to diagonal accesses), where the implementations for the first/second and pre-last/last circle should (probably?) be corrected. Needs to be verified.

If zero Dirichlet boundary conditions are prescribed, this bug has no effect though.