224 add openmp acceleration #225

ProkopHapala · 2023-11-03T16:43:54Z

Closes #224

I implemented OpenMP paralelization of forcefield gengeration and probe particle relaxation on CPU
I'm not sure it the dependency on -fopenmp in makefile and #include <omp.h> does not couase some problems on other platforms ?
For now I tested it only on Graphene example

…e specifically function interateGrid3D_omp() in Grid.h)

…in ProbeParticle.cpp and relaxedScan3D_omp() in HighLevel.py )

yakutovicha · 2023-11-05T11:42:05Z

@ProkopHapala please merge the main branch into the current one, to make sure the tests are passing.

Also, please enable the pre-commit.

ProkopHapala · 2023-11-06T10:00:57Z

@ProkopHapala please merge the main branch into the current one, to make sure the tests are passing.

But merging should be done only after the PR is accepted or not?
Also I think I should put there some option (switch, environment variable) to enable run it without OpenMP

Also, please enable the pre-commit.

I' tried, but not sure what is going on, why all tests are skipped?

prokop@DesktopGTX3060:~/git/ppafm$ git commit -a -m "try pre-commit"
[INFO] Initializing environment for https://github.com/pre-commit/pre-commit-hooks.
[INFO] Initializing environment for https://github.com/pycqa/isort.
[INFO] Initializing environment for https://github.com/PyCQA/autoflake.
[INFO] Initializing environment for https://github.com/asottile/pyupgrade.
[INFO] Initializing environment for https://github.com/psf/black.
[INFO] Initializing environment for https://github.com/jumanjihouse/pre-commit-hook-yamlfmt.
[INFO] Installing environment for https://github.com/pre-commit/pre-commit-hooks.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
[INFO] Installing environment for https://github.com/pycqa/isort.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
[INFO] Installing environment for https://github.com/PyCQA/autoflake.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
[INFO] Installing environment for https://github.com/asottile/pyupgrade.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
[INFO] Installing environment for https://github.com/psf/black.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
[INFO] Installing environment for https://github.com/jumanjihouse/pre-commit-hook-yamlfmt.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
fix end of files.....................................(no files to check)Skipped
trim trailing whitespace.............................(no files to check)Skipped
check yaml...........................................(no files to check)Skipped
check for added large files..........................(no files to check)Skipped
isort................................................(no files to check)Skipped
autoflake............................................(no files to check)Skipped
pyupgrade............................................(no files to check)Skipped
black................................................(no files to check)Skipped
Format YAML files....................................(no files to check)Skipped
On branch 224-add-openmp-acceleration
Your branch is up to date with 'origin/224-add-openmp-acceleration'.

nothing to commit, working tree clean
prokop@DesktopGTX3060:~/git/ppafm$

yakutovicha · 2023-11-06T12:06:31Z

But merging should be done only after the PR is accepted or not?

When a PR is merged we say "The current branch is merged into the main branch". To get the current branch up-to-date with main we "merge main into the current branch.":

$ git checkout 224-add-openmp-acceleration
$ git merge main

This is what I meant.

Also, please enable the pre-commit.

I' tried, but not sure what is going on, why all tests are skipped?

What you did is fine, but this only works for the files that were modified in the current commit. As far as I can see your current commit has no changes. The way to apply pre-commit at this point is to run it for the whole repository:

$ pre-commit run --all-files

And then commit the updates files.

ProkopHapala · 2023-11-06T15:41:23Z

$ pre-commit run --all-files
And then commit the updates files.

OK, I did.

It is doing this horrible thing. Can we prevent it?

(adding lines in between definition of ctypes binding of function and its arguments)

@@ -284,6 +290,8 @@ def getGaussDensity(Rs, cRAs):
 # void getSlaterDensity( int natoms_, double * Ratoms_, double * cRAs ){
 lib.getSlaterDensity.argtypes = [c_int, array2d, array2d]
 lib.getSlaterDensity.restype = None
+
+
 def getSlaterDensity(Rs, cRAs):
     natom = len(Rs)
     lib.getSlaterDensity(natom, Rs, cRAs)

yakutovicha · 2023-11-07T08:06:57Z

It is doing this horrible thing. Can we prevent it?

As far as I can tell the ways to prevent things are still the same:

Add fmt: off - fmt:on
Do not format this file at all (add it to the ignore section of black).

In that particular case, the best thing IMO is to move everything inside the python function:

 def getSlaterDensity(Rs, cRAs):
     # void getSlaterDensity( int natoms_, double * Ratoms_, double * cRAs ){
     lib.getSlaterDensity.argtypes = [c_int, array2d, array2d]
     lib.getSlaterDensity.restype = None
     natom = len(Rs)
     lib.getSlaterDensity(natom, Rs, cRAs)

ProkopHapala · 2023-11-07T14:49:18Z

@NikoOinonen @yakutovicha

ad Some checks were not successful ... when I look on the details of the unsuccesful test, it seems to be something conected with OpenCL and reikna, which should be completely orthogonal to the CPU code in C++ I eddited. Isn't this actually symptom of another issue which @NikoOinonen raised some time ago ?

Run pytest tests -v --cov
============================= test session starts ==============================
platform linux -- Python 3.9.18, pytest-7.4.3, pluggy-1.3.0 -- /opt/hostedtoolcache/Python/3.9.18/x64/bin/python
cachedir: .pytest_cache
rootdir: /home/runner/work/ppafm/ppafm
plugins: cov-4.1.0
/opt/hostedtoolcache/Python/3.9.18/x64/lib/python3.9/site-packages/coverage/report_core.py:[11](https://github.com/Probe-Particle/ppafm/actions/runs/6773029204/job/18406914962?pr=225#step:5:12)5: CoverageWarning: Couldn't parse '/home/runner/work/ppafm/ppafm/_opt_hostedtoolcache_Python_3_9_18_x64_lib_python3_9_site_packages_reikna_algorithms_reduce_mako': No source for code: '/home/runner/work/ppafm/ppafm/_opt_hostedtoolcache_Python_3_9_18_x64_lib_python3_9_site_packages_reikna_algorithms_reduce_mako'. (couldnt-parse)
  coverage._warn(msg, slug="couldnt-parse")

NikoOinonen · 2023-11-07T15:03:39Z

@ProkopHapala The fix is on the main branch. If you merge the main branch into this branch, the tests should pass. @yakutovicha was mentioning about this above:

$ git checkout 224-add-openmp-acceleration
$ git merge main

…3d in relaxedScan3D_omp was not implemented)

ProkopHapala · 2023-11-07T15:20:43Z

@ProkopHapala The fix is on the main branch. If you merge the main branch into this branch, the tests should pass. @yakutovicha was mentioning about this above:

$ git checkout 224-add-openmp-acceleration
$ git merge main

Yes, I did it. Still the tests are failing for some reason (which seems unrelated to my changes in this branch)
But the ubuntu docker finished fine now.

NikoOinonen · 2023-11-07T15:24:54Z

I don't see the fix on this branch. This line is still the old one:

ppafm/.github/workflows/ci.yml

Line 24 in 916f3af

sudo apt install pocl-opencl-icd

whereas on the main branch it's this:

ppafm/.github/workflows/ci.yml

Line 24 in 9f009ae

pip install pocl-binary-distribution

Try the merge again. Maybe you just forgot to push.

NikoOinonen · 2023-11-07T15:28:11Z

Or maybe you have an old version of the main branch. I think you need to first checkout and pull the main branch before merging it to this branch.

ProkopHapala · 2023-11-07T18:23:27Z

checkout and pull the main branch before merging it to this branch.

yeh, I think this was the problem

ProkopHapala · 2023-11-08T07:55:46Z

Now it works fine. Thanks @NikoOinonen

Before we merge it with main branch it would be good to have some option to switch off the OpenMP.
There are three places:

CPPFLAGS= -fPIC -std=c++11 -O2 -mtune=native -fopenmp
in Makefile

#include <omp.h>
in ProbeParticle.cpp

if bOpenMP:
     fzs, PPpos = relaxedScan3D(xTips, yTips, zTips, trj=trj, bF3d=PPU.params["tiltedScan"])
else:
     fzs, PPpos = relaxedScan3D_omp(xTips, yTips, zTips, trj=trj, bF3d=PPU.params["tiltedScan"])

in HighLevel.py

NikoOinonen · 2023-11-08T09:34:59Z

I went ahead and fixed it to work also on Windows.

NikoOinonen · 2023-11-08T09:37:01Z

Before we merge it with main branch it would be good to have some option to switch off the OpenMP.

Is this necessary? As far as I can tell, you can control the number of threads with the OMP_NUM_THREADS variable, so you could set it to 1 to run single threaded, and then it's the same as before?

ProkopHapala · 2023-11-08T09:46:57Z

As far as I can tell, you can control the number of threads with the OMP_NUM_THREADS variable, so you could set it to 1 to run single threaded, and then it's the same as before?

perhaps it is not necessary. I'm just not completely sure if

every potential user has OpenMP library (perhaps yes?)
I did not test it troughfully. Maybe it would be good to check if OMP paralelized function gives the same results as original serial function (perhaps yes)

ProkopHapala · 2023-11-08T09:49:45Z

Another thing:

when using OpenMP paralelization, even on CPU the most time-consuming part become

1 save & load .xsf files
- Perhaps it would make sense save .npy by default
- or even make some script which does not save temporary file
plotting of images with matplotlib

NikoOinonen · 2023-11-08T10:30:04Z

every potential user has OpenMP library (perhaps yes?)

At least on my systems, I did not have to install anything extra, but it could be that I have it from some previously installed software. At least for compilation, it's only required for developers, so I would not worry. But maybe the there is some runtime library that is also required on the system? Is it possible to just check at runtime and then fall back on the non-OMP implementation if it's not available?

Perhaps it would make sense save .npy by default

.npy is good for speed, but not for visualizing the intermediate results, which could be more important as the default. We have the OpenCL anyways for those who really care about speed.

plotting of images with matplotlib

Could this be also parallelized? IO should not be a bottleneck.

ondrejkrejci · 2023-11-14T10:27:55Z

@ProkopHapala - Please add the documentation to wiki, then it is free to go from my point of view.

NikoOinonen

@ProkopHapala The code is good, just make sure to add the documentation as well when you merge.

ProkopHapala · 2023-11-15T20:41:20Z

@ProkopHapala The code is good, just make sure to add the documentation as well when you merge.

OK, I did Here
https://github.com/Probe-Particle/ppafm/wiki/Install-ppafm#openmp

for more information, see https://pre-commit.ci

codecov · 2023-11-15T20:54:33Z

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (c262867) 46.93% compared to head (87a9a0e) 46.46%.

Files	Patch %	Lines
ppafm/PPPlot.py	62.50%	3 Missing ⚠️
ppafm/cpp_utils.py	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #225      +/-   ##
==========================================
- Coverage   46.93%   46.46%   -0.48%     
==========================================
  Files          35       35              
  Lines        5147     5178      +31     
==========================================
- Hits         2416     2406      -10     
- Misses       2731     2772      +41

Flag	Coverage Δ
python-3.10	`46.47% <89.18%> (-0.48%)`	⬇️
python-3.11	`46.44% <89.18%> (-0.48%)`	⬇️
python-3.12	`46.44% <89.18%> (-0.48%)`	⬇️
python-3.7	`46.26% <89.18%> (-0.48%)`	⬇️
python-3.9	`46.36% <89.18%> (-0.48%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

forgot to comment old non-OpenMP relaxedScan3D() call

for more information, see https://pre-commit.ci

ondrejkrejci · 2023-11-15T21:08:47Z

I have went through the Wiki text, and added a sentence about setting the number of cores.

ProkopHapala · 2023-11-15T21:11:01Z

@yakutovicha What does this mean ?

Some checks were not successful
codecov/project — 46.46% (-0.48%) compared to c262867
?

yakutovicha · 2023-11-15T21:17:46Z

Some checks were not successful
codecov/project — 46.46% (-0.48%) compared to c262867

Literally what it says - you added a few lines that are not executed during tests, so the coverage has decreased in comparison to the main branch. However, as we discussed with @NikoOinonen - we should disable the failure because of this. It is too strict at the moment.

yakutovicha · 2023-11-15T21:19:42Z

You can go ahead and merge the PR. We will later configure codecov not to fail due to lower coverage.

yakutovicha · 2023-11-15T21:32:31Z

I guess I've just set the threshold to 2%, @ProkopHapala could you try to push something small here? Hopefully, this should pass now.

NikoOinonen · 2023-11-16T08:27:45Z

It's passing on the main branch so should be fine now.

ProkopHapala added 2 commits November 3, 2023 12:10

made OpenMP paralelized verzion of Force-Field generation on CPU (mor…

cd18b46

…e specifically function interateGrid3D_omp() in Grid.h)

OpenMP paralelization of relaxed_scan on CPU ( relaxTipStrokes_omp() …

3dd0886

…in ProbeParticle.cpp and relaxedScan3D_omp() in HighLevel.py )

ProkopHapala linked an issue Nov 3, 2023 that may be closed by this pull request

add OpenMP acceleration? #224

Closed

ProkopHapala requested review from yakutovicha and NikoOinonen November 3, 2023 16:54

precomit

b80e4fb

corrected error causing crashing of examples/PTCDA_single/ (option bF…

916f3af

…3d in relaxedScan3D_omp was not implemented)

Merge branch 'main' into 224-add-openmp-acceleration

9d8c9aa

Fix OpenMP for Windows.

7eff17b

NikoOinonen approved these changes Nov 15, 2023

View reviewed changes

Merge branch 'main' into 224-add-openmp-acceleration

9244182

[pre-commit.ci] auto fixes from pre-commit.com hooks

ed0d431

for more information, see https://pre-commit.ci

ProkopHapala and others added 2 commits November 15, 2023 21:56

Update HighLevel.py

9855f20

forgot to comment old non-OpenMP relaxedScan3D() call

[pre-commit.ci] auto fixes from pre-commit.com hooks

87a9a0e

for more information, see https://pre-commit.ci

ProkopHapala merged commit 80e79a0 into main Nov 15, 2023

ProkopHapala deleted the 224-add-openmp-acceleration branch November 15, 2023 21:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

224 add openmp acceleration #225

224 add openmp acceleration #225

ProkopHapala commented Nov 3, 2023 •

edited by NikoOinonen

Loading

yakutovicha commented Nov 5, 2023

ProkopHapala commented Nov 6, 2023

yakutovicha commented Nov 6, 2023

ProkopHapala commented Nov 6, 2023

yakutovicha commented Nov 7, 2023 •

edited

Loading

ProkopHapala commented Nov 7, 2023

NikoOinonen commented Nov 7, 2023

ProkopHapala commented Nov 7, 2023

NikoOinonen commented Nov 7, 2023

NikoOinonen commented Nov 7, 2023

ProkopHapala commented Nov 7, 2023

ProkopHapala commented Nov 8, 2023

NikoOinonen commented Nov 8, 2023

NikoOinonen commented Nov 8, 2023

ProkopHapala commented Nov 8, 2023

ProkopHapala commented Nov 8, 2023

NikoOinonen commented Nov 8, 2023

ondrejkrejci commented Nov 14, 2023 •

edited

Loading

NikoOinonen left a comment

ProkopHapala commented Nov 15, 2023

codecov bot commented Nov 15, 2023 •

edited

Loading

ondrejkrejci commented Nov 15, 2023

ProkopHapala commented Nov 15, 2023

yakutovicha commented Nov 15, 2023 •

edited

Loading

yakutovicha commented Nov 15, 2023

yakutovicha commented Nov 15, 2023

NikoOinonen commented Nov 16, 2023

224 add openmp acceleration #225

224 add openmp acceleration #225

Conversation

ProkopHapala commented Nov 3, 2023 • edited by NikoOinonen Loading

yakutovicha commented Nov 5, 2023

ProkopHapala commented Nov 6, 2023

yakutovicha commented Nov 6, 2023

ProkopHapala commented Nov 6, 2023

yakutovicha commented Nov 7, 2023 • edited Loading

ProkopHapala commented Nov 7, 2023

NikoOinonen commented Nov 7, 2023

ProkopHapala commented Nov 7, 2023

NikoOinonen commented Nov 7, 2023

NikoOinonen commented Nov 7, 2023

ProkopHapala commented Nov 7, 2023

ProkopHapala commented Nov 8, 2023

NikoOinonen commented Nov 8, 2023

NikoOinonen commented Nov 8, 2023

ProkopHapala commented Nov 8, 2023

ProkopHapala commented Nov 8, 2023

NikoOinonen commented Nov 8, 2023

ondrejkrejci commented Nov 14, 2023 • edited Loading

NikoOinonen left a comment

Choose a reason for hiding this comment

ProkopHapala commented Nov 15, 2023

codecov bot commented Nov 15, 2023 • edited Loading

Codecov Report

ondrejkrejci commented Nov 15, 2023

ProkopHapala commented Nov 15, 2023

yakutovicha commented Nov 15, 2023 • edited Loading

yakutovicha commented Nov 15, 2023

yakutovicha commented Nov 15, 2023

NikoOinonen commented Nov 16, 2023

ProkopHapala commented Nov 3, 2023 •

edited by NikoOinonen

Loading

yakutovicha commented Nov 7, 2023 •

edited

Loading

ondrejkrejci commented Nov 14, 2023 •

edited

Loading

codecov bot commented Nov 15, 2023 •

edited

Loading

yakutovicha commented Nov 15, 2023 •

edited

Loading