Skip to content
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

McMurchie-Davidson integrals for angular momentum #2483

Merged
merged 10 commits into from
Mar 21, 2022
Merged

McMurchie-Davidson integrals for angular momentum #2483

merged 10 commits into from
Mar 21, 2022

Conversation

maxscheurer
Copy link
Contributor

@maxscheurer maxscheurer commented Mar 16, 2022 •
edited
Loading

Description

This PR refactors angular momentum integrals (AngularMomentumInt) using the McMurchie-Davidson scheme.

It is the first PR of a series to replace most of the existing OS86 code with M-D (#2414). I've chosen the angular momentum integrals because they are really simple and only require the E matrix (Hermite-to-Cartesian conversion factors).

Todos

  • AngularMomentumInt w/ M-D scheme

Questions

  • Is it worth to pre-allocate the E-matrix? This would require some boilerplate code, which could be worth packing into a small class that all future M-D integrals will inherit from?

Checklist

Status

  • Ready for review
  • Ready for merge

@maxscheurer maxscheurer mentioned this pull request Mar 16, 2022
Closed
@JonathonMisiewicz JonathonMisiewicz added this to the Psi4 1.6 milestone Mar 16, 2022
@maxscheurer
Copy link
Contributor Author

Apparently, gcc6 on CI does not like structured bindings? I thought C++17 was fully supported 😞

@andysim
Copy link
Member

andysim commented Mar 16, 2022

Yeah, if we're going to allow C++17 we should upgrade the minimum GCC to version 7 in our CI.

@jturney
Copy link
Member

jturney commented Mar 16, 2022

Yeah, I would preallocate the E in the constructor and then simply fill it with zeroes in the compute pair function.

jturney
jturney approved these changes Mar 21, 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 super nice, too.

andysim
andysim reviewed Mar 21, 2022
View reviewed changes
psi4/src/psi4/libmints/angularmomentum.cc Outdated

ao12++;
}
for (auto& [l1, m1, n1, index1] : comps_am1) {
Copy link
Member

Choose a reason for hiding this comment

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

these two range based for loops should use const auto &, as the loop variables are read-only

andysim
andysim approved these changes Mar 21, 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.

Thanks for taking the time to do this - I think it's an excellent cleanup, and it also provides some very useful machinery for other OEI types. Kudos for the pytests - they will help us greatly as we continue to clean up the other integrals. LGTM!

loriab
loriab approved these changes Mar 21, 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.

a few optional changes. lgtm!

cmake/custom_cxxstandard.cmake Outdated Show resolved Hide resolved
cmake/custom_cxxstandard.cmake Outdated Show resolved Hide resolved
.azure-pipelines/azure-pipelines-linux.yml Show resolved Hide resolved
tests/pytests/test_mcmurchie_davidson.py Outdated Show resolved Hide resolved
psi4/src/psi4/libmints/angularmomentum.cc Outdated Show resolved Hide resolved
psi4/src/psi4/libmints/mcmurchiedavidson.cc Outdated Show resolved Hide resolved
@maxscheurer
Copy link
Contributor Author

I'll make the requested changes and squash-merge when CI passes. 👍 Thanks everyone for the reviews!

@maxscheurer maxscheurer merged commit c21c74c into psi4:master Mar 21, 2022
@maxscheurer maxscheurer deleted the angmom_md branch March 21, 2022 22:01
@loriab loriab added the Libint2 label Mar 23, 2022
@loriab loriab mentioned this pull request Mar 23, 2022
Open
zachglick pushed a commit to zachglick/psi4 that referenced this pull request Apr 18, 2022
@maxscheurer @zachglick
McMurchie-Davidson integrals for angular momentum (psi4#2483)
dfba9f1
zachglick pushed a commit to zachglick/psi4 that referenced this pull request Apr 18, 2022
@maxscheurer @zachglick
McMurchie-Davidson integrals for angular momentum (psi4#2483)
6b3b5ac
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@loriab loriab loriab approved these changes

@susilehtola susilehtola susilehtola left review comments

@andysim andysim andysim approved these changes

@jturney jturney jturney 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.
6 participants
@maxscheurer @andysim @jturney @susilehtola @loriab @JonathonMisiewicz