Parilu omp

[hartwiganzt]

This pull request adds the ParILU functionality for the OMP executor. A challenge are the unit tests: the results will - depending on the parallelism - be very different from the reference executor results. Currently, I set a threshold of 0.3 for this specific test case.

[tcojean]

If you do a huge amount of sweeps (say 100, could be less) is the precision better? In which case, we may prefer severely increasing the amount of sweeps specifically for the tests to ensure we have actual close values. Currently, 0.3 is very high as it is the order of most values in your test matrix. Values could be more than double the actual ones.

[codecov]

Codecov Report

Merging #315 into develop will increase coverage by 0.04%.
The diff coverage is 98.63%.

@@             Coverage Diff             @@
##           develop     #315      +/-   ##
===========================================
+ Coverage    98.17%   98.21%   +0.04%     
===========================================
  Files          223      224       +1     
  Lines        17180    17321     +141     
===========================================
+ Hits         16866    17012     +146     
+ Misses         314      309       -5

Impacted Files	Coverage Δ
omp/factorization/par_ilu_kernels.cpp	100% <100%> (+100%)	⬆️
omp/test/factorization/par_ilu_kernels.cpp	96.92% <96.92%> (ø)
core/base/mtx_io.cpp	93.5% <0%> (+0.64%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8bf33e0...80b6ce1. Read the comment docs.

[hartwiganzt]

It is not easy to say whether more sweeps increase the quality. In fact, the more sweeps, the higher is the chance of breakdown because of division-by-zero. For reference I below show a table with top-level iteration counts taken from the [ISC paper]((https://link.springer.com/chapter/10.1007/978-3-319-20119-1_1)
The results indicate that 3 sweeps may be a good choice....

[thoasm]

I am strongly in favor of using a file instead of this big, hard-coded matrix. Using a file has several benefits:

• The code looks a lot nicer (no need for clang-format off / on)
• We probably have more precise values (if you use the matrix market file)
• Checking if the behavior is expected is a lot easier with a file since you can also read it with matlab
• You can make changes inside the file a lot easier compared to inside the code
• You can swap the matrix without a lot of effort

You can use the following patch to read the matrix from file (you have to copy the matrix to omp/test/factorization/ani1.mtx in order for this code to work):
patch_read_matrix_from_file.txt
(Note: All changes after the newly added SetUp() function are style changes caused by make format on my system)

We have one draw-back with the implementation I am proposing: we lose the ability to launch the test wherever we want because we have to be in the local directory to find the matrix-file.

We could get around this by creating a header file for the tests where we write the path of the build directory from cmake.
I think the proposed version should be fine, especially since there is an error message that explains why it crashes if it is called from the wrong place. Additionally, make test also works with this implementation.
But if it is requested, I can look into how to generate the header where the build directory is stored (or for a better alternative).

[thoasm]

I would like the code duplication to be reduced by creating functions in the test.

[thoasm]

Looks good, just two small things.

[thoasm]

LGTM!

[tcojean]

LGTM. I only have small comments.

In addition, I tried to see if there was any obvious way to improve the parallelization, but that was not fruitful. On the other end, making a reduction out of compute_nnz_l_u does not really worsen the computation time (on big enough matrices), although it does not really improve it either. You can decide whether you want to do it or not.