search

dftbplus / skprogs

Basic programs for generating Slater-Koster files for the DFTB-method
GNU Lesser General Public License v3.0
25 stars 19 forks source link

Parallelised version with LibXC 5.x interface #4

Closed tsihyoung closed 2 years ago

tsihyoung commented 2 years ago
  1. Accommodate to LibXC 5.x interface;
  2. Support float confinement powers (by replacing factorial to gamma function);
  3. Change type conversion from float to dble;
  4. Support OpenMP.
awvwgk commented 2 years ago

Thanks for sharing. It would be preferable to split this patch in separate PRs for each of the proposed features and fixes. Especially, the libxc update will be something we can merge quickly, while the parallelization might need a more detailed review.

tsihyoung commented 2 years ago

Thanks for your reply. However, it would be tedious to split these patches since I only committed once in my fork.

  1. For the accuracy suspicion, I have tested several parameters, the maximum deviation between this and previous commits is about 1E-12. The only thing I am not sure is the replacement of factorial by Gamma function.
  2. For the parallelisation, I only implemented OpenMP, so it can be easily disabled by not passing -qopenmp to the compiler.
bhourahine commented 2 years ago

@tsihyoung unfortunately complex pull requests with several unrelated parts are usually considered not to be a good idea in software development, as this causes problems with 1. reviewing at high quality, 2. bisection and localization of problems if bug hunting and 3. usually take more time to get incorporated.

bhourahine commented 2 years ago

@tsihyoung I have started splitting the PR as separate items, which should speed up them being incorporated (also due to the recent changes merged from #3 there will probably also need to be changes about modules).

See the progress so far in my repository. These use your email and name for the commit, so please a) check they work as intended or if there are needed changes b) either copy and make separate pull requests, or let me know if you want me to make pull requests for these (we will then need to collaborate about requested changes).

There is one significant change not mentioned in your log message from your original PRs, switching from gnu to intel compilers, that probably should be discussed (the correct thing would be to support both).

Additionally, and this is a general problem with the skprogs code base outside of your changes, the real( ,kind=dp) structure is better to use in Fortran instead of double or d0.

Finally, for long log messages on commits, it is useful if a short title line can be used, with the additional details then on the lines afterwards.

Regards Ben

tsihyoung commented 2 years ago

@bhourahine Really thanks for your nice commits. I am sorry for the extra burdens caused by me.

I am working on splitting the PRs and adapting recent commits. I noticed that you have changed LibXC interface from F90 to F03 in your branch LibXC5_f03, which should work well with LibXC 4.x also (not tested, but the API exists in v4.x). We may need to test and revise the README later.

Best regards

bhourahine commented 2 years ago

@tsihyoung thank you for splitting up the pull requests.

Yes, the libXC developers offered the f03 interface in the 4.x series, then removed it, finally added it back in the 5.x series. Their configure script currently seem to build the f03 interface preferentially.

For future pull requests, another way to simplify getting these merged would be to develop features in separate branches, as we tend to use this workflow (see the developer page ).