Skip to content

Multipole Potential Integrals with McMurchie-Davidson #2504

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
maxscheurer merged 10 commits into from
Mar 30, 2022
Merged

Multipole Potential Integrals with McMurchie-Davidson #2504

maxscheurer merged 10 commits into from
Mar 30, 2022

Conversation

@maxscheurer
Copy link
Contributor

@maxscheurer maxscheurer commented Mar 25, 2022 •
edited
Loading

Description

This is the third PR of the McMurchie-Davidson series, now tackling MultipolePotentialInt (used for PE/EFP).

I'm not happy with the performance yet, so I'll do some profiling to see what could be improved.

Todos

Blurb for the Release Notes

  • Added the ability to compute integral representations of the derivative of the Coulomb operator (potential, field, field gradient, etc.), with arbitrary derivative order; the previous Obara-Saika based code was limited to third order derivatives.

Checklist

Status

  • Ready for review
  • Ready for SQUASH-merge

@maxscheurer maxscheurer marked this pull request as ready for review March 29, 2022 09:15
@maxscheurer
Copy link
Contributor Author

maxscheurer commented Mar 29, 2022 •
edited
Loading

Small benchmark

The performance issues are resolved. 🎉 As in #2496, only for small basis set angular momentum, the old code is faster than M-D, but for larger basis set/angmom, M-D outperforms the OS code.

Performed as in #2496. Here, the molecule is para-nitroaniline (typical example for EFP/PE 😄).

Results

basis order M-D [ms] OS [ms] ratio M-D/OS
cc-pvdz (nbf = 170) 0 5.75 5.45 1.05
1 9.55 7.72 1.24
2 17.69 12.06 1.47
3 30.41 19.61 1.55
cc-pvtz (nbf = 384) 0 14.2 14.02 1.01
1 27.75 25.62 1.08
2 55.37 50.41 1.1
3 99.74 89.52 1.1
cc-pvqz (nbf = 730) 0 43.66 45.69 0.96
1 94.84 101.92 0.93
2 197.61 222.04 0.89
3 366.13 428.15 0.85
cc-pv5z (nbf = 1240) 0 132.84 160.85 0.83
1 323.07 406.33 0.8
2 694.14 942.28 0.74
3 1291.59 2011.75 0.64

@maxscheurer maxscheurer requested review from andysim and loriab March 29, 2022 10:57
JonathonMisiewicz
JonathonMisiewicz reviewed Mar 29, 2022
View reviewed changes
Copy link
Contributor

@JonathonMisiewicz JonathonMisiewicz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple minor comments from me. I'd be happy to approve, but you're probably better off with reviews from Lori, Andy, and Jet.

loriab
loriab approved these changes Mar 29, 2022
View reviewed changes
Copy link
Member

@loriab loriab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice test! I'd consider myself a gray checkmark, but lgtm!

@maxscheurer maxscheurer requested a review from jturney March 29, 2022 14:46
jturney
jturney approved these changes Mar 29, 2022
View reviewed changes
Copy link
Member

@jturney jturney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

andysim
andysim reviewed Mar 29, 2022
View reviewed changes
Copy link
Member

@andysim andysim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will try to pull this and test with Intel compilers. We know that all files that include L2's engine.h need C++14 instead of C++17 with Intel, but it's possible that the real culprit is the downstream boys.h that you are using here. I don't think it is, but it's worth checking and an easy fix if it turns out to be the case.

zachglick
zachglick approved these changes Mar 29, 2022
View reviewed changes
Copy link
Contributor

@zachglick zachglick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great!

andysim
andysim reviewed Mar 29, 2022
View reviewed changes
@loriab loriab added this to the Psi4 1.6 milestone Mar 30, 2022
@loriab loriab added the Libint2 label Mar 30, 2022
@maxscheurer maxscheurer requested a review from andysim March 30, 2022 15:22
andysim
andysim reviewed Mar 30, 2022
View reviewed changes
andysim
andysim approved these changes Mar 30, 2022
View reviewed changes
Copy link
Member

@andysim andysim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another fantastic effort! It's great to see negative lines of code, especially with so much new functionality. LGTM!

@maxscheurer maxscheurer merged commit b7223ca into psi4:master Mar 30, 2022
@maxscheurer maxscheurer deleted the mulpot_md branch March 30, 2022 15:57
This was referenced Apr 5, 2022
Open
Closed
zachglick pushed a commit to zachglick/psi4 that referenced this pull request Apr 18, 2022
@maxscheurer @andysim @zachglick
Multipole Potential Integrals with McMurchie-Davidson (psi4#2504)
0959488 
* multipole potential integrals with M-D

* some loop unrolling

* performance improvements

* only zero out required parts of buffers

* cleanup, remove unused vars, simplify cca comps

* more cleanup, more eq numbers

* reviews, back to std::fill, fix psi4#2499

* fixes for Intel

* Update psi4/src/psi4/libmints/CMakeLists.txt

Co-authored-by: Andy Simmonett <[email protected]>

* more reviews

Co-authored-by: Andy Simmonett <[email protected]>
zachglick pushed a commit to zachglick/psi4 that referenced this pull request Apr 18, 2022
@maxscheurer @andysim @zachglick
Multipole Potential Integrals with McMurchie-Davidson (psi4#2504)
760968a 
* multipole potential integrals with M-D

* some loop unrolling

* performance improvements

* only zero out required parts of buffers

* cleanup, remove unused vars, simplify cca comps

* more cleanup, more eq numbers

* reviews, back to std::fill, fix psi4#2499

* fixes for Intel

* Update psi4/src/psi4/libmints/CMakeLists.txt

Co-authored-by: Andy Simmonett <[email protected]>

* more reviews

Co-authored-by: Andy Simmonett <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hokru hokru hokru left review comments

@zachglick zachglick zachglick approved these changes

@JonathonMisiewicz JonathonMisiewicz JonathonMisiewicz left review comments

@jturney jturney jturney approved these changes

@andysim andysim andysim approved these changes

@loriab loriab loriab approved these changes

Assignees

No one assigned

Labels

Libint2

Projects

None yet

Milestone

Psi4 1.6

Development

Successfully merging this pull request may close these issues.

7 participants

@maxscheurer @jturney @andysim @hokru @loriab @zachglick @JonathonMisiewicz