Skip to content
This repository has been archived by the owner on Dec 8, 2024. It is now read-only.

FCHL19 with periodic boundary conditions and OpenMP. #137

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from

Conversation

kvkarandashev
Copy link

FCHL19 now works with periodic boundary conditions and is parallelized with respect to atoms.

Minor OpenMP addition in kernels/fgradient_kernels.f90.

@kvkarandashev kvkarandashev changed the base branch from master to develop February 28, 2021 15:43
@andersx
Copy link
Member

andersx commented Mar 4, 2021

Fantastic contribution, thank you so much for this!

  • I don't know why that OMP parallel loop was only running in serial, thanks for fixing that
  • I am also not sure why the CI is not running properly, I will investigate!
  • In the meantime could you add your name to the headers of the files you've changed? I'll also add you as co-author on the qmlcode.org website

Thanks again, this is awesome!

@kvkarandashev
Copy link
Author

Thank you for the reply!

  • The commented-out draft of OpenMP parallelization included applying a "REDUCTION" clause to arrays whose size scale as natoms and natoms^2, which can drastically increase memory requirements during parallel runs; I suspect that's why it wasn't used. I ended up allocating a separate temporary array with a size scaling as natoms^2 to avoid the problem.
  • I've updated the file headers.

@andersx
Copy link
Member

andersx commented Mar 9, 2021

Thank you for the update! I'll review this tonight, so should be able to merge this in!

@kvkarandashev
Copy link
Author

Great! Last minute update that I decided to make: I added a revised test for PBC implementation of FCHL19, the script "test_fchl_acsf_pbc.py". Before this update the fork contained first draft of the test created by Max Schwilk from benchmarks I used earlier to check that the code works correctly (it's integrated into test_fchl_acsf_force.py script and adds some LiH* files to data); I hope the duplication does not create too much confusion.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants