224 add openmp acceleration

ProkopHapala commented 10 months ago

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

yakutovicha commented 10 months ago

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

Also, please enable the pre-commit.

ProkopHapala commented 10 months ago

@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 commented 10 months ago

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 commented 10 months ago

$ 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 commented 10 months ago

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 commented 10 months ago

@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 commented 10 months ago

@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

ProkopHapala commented 10 months ago

@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 commented 10 months ago

I don't see the fix on this branch. This line is still the old one: https://github.com/Probe-Particle/ppafm/blob/916f3afbe6c5c4b38a2ee2cce46cb188223c79bd/.github/workflows/ci.yml#L24 whereas on the main branch it's this: https://github.com/Probe-Particle/ppafm/blob/9f009aec25d8d895f4168f061501211a37a9dd1c/.github/workflows/ci.yml#L24 Try the merge again. Maybe you just forgot to push.

NikoOinonen commented 10 months ago

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 commented 10 months ago

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

yeh, I think this was the problem

ProkopHapala commented 10 months ago

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 commented 10 months ago

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

NikoOinonen commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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

ProkopHapala commented 10 months ago

@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

codecov[bot] commented 10 months ago

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 :warning:
ppafm/cpp_utils.py	0.00%	1 Missing :warning:

Additional details and impacted files

```diff @@ 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](https://app.codecov.io/gh/Probe-Particle/ppafm/pull/225/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Probe-Particle) | Coverage Δ | | |---|---|---| | [python-3.10](https://app.codecov.io/gh/Probe-Particle/ppafm/pull/225/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Probe-Particle) | `46.47% <89.18%> (-0.48%)` | :arrow_down: | | [python-3.11](https://app.codecov.io/gh/Probe-Particle/ppafm/pull/225/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Probe-Particle) | `46.44% <89.18%> (-0.48%)` | :arrow_down: | | [python-3.12](https://app.codecov.io/gh/Probe-Particle/ppafm/pull/225/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Probe-Particle) | `46.44% <89.18%> (-0.48%)` | :arrow_down: | | [python-3.7](https://app.codecov.io/gh/Probe-Particle/ppafm/pull/225/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Probe-Particle) | `46.26% <89.18%> (-0.48%)` | :arrow_down: | | [python-3.9](https://app.codecov.io/gh/Probe-Particle/ppafm/pull/225/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Probe-Particle) | `46.36% <89.18%> (-0.48%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Probe-Particle#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

ondrejkrejci commented 10 months ago

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

ProkopHapala commented 10 months ago

@yakutovicha What does this mean ?

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

yakutovicha commented 10 months ago

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 commented 10 months ago

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

yakutovicha commented 10 months ago

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 commented 10 months ago

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

Probe-Particle / ppafm

224 add openmp acceleration #225

Codecov Report