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.

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

Add multipole moment integrals to hf #2166

Merged
merged 27 commits into from
Feb 18, 2022
Merged

Conversation

soranjh
Copy link
Contributor

@soranjh soranjh commented Feb 3, 2022

Context:
This PR adds functions required for computing multipole moment integral over two contracted Gaussian functions. These integrals are needed to construct multipole moment observables.

Description of the Change:
The functions _hermite_moment, gaussian_moment, and moment_integral are added to hf.ientegrals.

Benefits:
This functionality is required for computing dipole moment without using external qchem libraries, which are currently used by pennylane-qchem.

Possible Drawbacks:
NA

Related GitHub Issues:
NA

@soranjh soranjh added WIP 🚧 Work-in-progress qchem ⚛️ Related to the QChem package labels Feb 3, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Feb 3, 2022

Hello. You may have forgotten to update the changelog!
Please edit doc/releases/changelog-dev.md with:

  • A one-to-two sentence description of the change. You may include a small working example for new features.
  • A link back to this PR.
  • Your name (or GitHub username) in the contributors section.

@codecov
Copy link

codecov bot commented Feb 3, 2022

Codecov Report

Merging #2166 (e20cac4) into qchem_refactor (4fa5fb5) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@               Coverage Diff               @@
##           qchem_refactor    #2166   +/-   ##
===============================================
  Coverage           99.25%   99.25%           
===============================================
  Files                 233      233           
  Lines               18195    18227   +32     
===============================================
+ Hits                18060    18092   +32     
  Misses                135      135           
Impacted Files Coverage Δ
pennylane/hf/__init__.py 100.00% <ø> (ø)
pennylane/hf/integrals.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4fa5fb5...e20cac4. Read the comment docs.

@soranjh soranjh changed the title [WIP] add integral for computing multipole moment Add functions for computing multipole moment integrals Feb 4, 2022
@soranjh soranjh removed the WIP 🚧 Work-in-progress label Feb 4, 2022
@soranjh soranjh marked this pull request as ready for review February 4, 2022 19:48
@soranjh soranjh changed the title Add functions for computing multipole moment integrals Add multipole moment integrals to hf Feb 4, 2022
@ixfoduap ixfoduap self-requested a review February 8, 2022 18:46
Copy link
Contributor

@ixfoduap ixfoduap left a comment

Choose a reason for hiding this comment

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

Overall looks great! Awesome job getting this ready so quickly

I left basically just minor comments. Let's use this PR to already make better choices for function naming

pennylane/hf/__init__.py Outdated Show resolved Hide resolved
pennylane/hf/integrals.py Outdated Show resolved Hide resolved
pennylane/hf/integrals.py Outdated Show resolved Hide resolved
pennylane/hf/integrals.py Outdated Show resolved Hide resolved
pennylane/hf/integrals.py Outdated Show resolved Hide resolved
pennylane/hf/integrals.py Outdated Show resolved Hide resolved
pennylane/hf/integrals.py Show resolved Hide resolved
pennylane/hf/integrals.py Show resolved Hide resolved
tests/hf/test_integrals.py Show resolved Hide resolved
tests/hf/test_integrals.py Show resolved Hide resolved
pennylane/hf/integrals.py Outdated Show resolved Hide resolved
Copy link
Contributor

@ixfoduap ixfoduap left a comment

Choose a reason for hiding this comment

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

It's a minor thing, but consider writing a better explanation for the return of the hermite_moment function

pennylane/hf/integrals.py Outdated Show resolved Hide resolved
pennylane/hf/integrals.py Outdated Show resolved Hide resolved
pennylane/hf/integrals.py Outdated Show resolved Hide resolved
Copy link
Contributor

@agran2018 agran2018 left a comment

Choose a reason for hiding this comment

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

Thanks @soranjh. Looks good on my side. I have just left few comments regarding notation.

pennylane/hf/integrals.py Outdated Show resolved Hide resolved
pennylane/hf/integrals.py Outdated Show resolved Hide resolved
pennylane/hf/integrals.py Outdated Show resolved Hide resolved
Copy link
Contributor

@obliviateandsurrender obliviateandsurrender 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 to go.

pennylane/hf/integrals.py Show resolved Hide resolved
pennylane/hf/integrals.py Outdated Show resolved Hide resolved
pennylane/hf/integrals.py Outdated Show resolved Hide resolved
Copy link
Contributor

@Jaybsoni Jaybsoni left a comment

Choose a reason for hiding this comment

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

Just a few stylistic changes, but other then that It looks good to me 👍🏼

pennylane/hf/integrals.py Outdated Show resolved Hide resolved
pennylane/hf/integrals.py Outdated Show resolved Hide resolved
pennylane/hf/integrals.py Outdated Show resolved Hide resolved
pennylane/hf/integrals.py Outdated Show resolved Hide resolved
pennylane/hf/integrals.py Show resolved Hide resolved
tests/hf/test_integrals.py Outdated Show resolved Hide resolved
@soranjh soranjh merged commit 025b33e into qchem_refactor Feb 18, 2022
@soranjh soranjh deleted the qchem_moment_integral branch February 18, 2022 20:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
qchem ⚛️ Related to the QChem package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants