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

Handle orbital overlap in spin operators #1275

Merged
merged 10 commits into from
Nov 15, 2023

Conversation

mrossinek
Copy link
Member

@mrossinek mrossinek commented Nov 14, 2023

Summary

Fixes #1273. For more details, please refer to that issue.

Details and comments

  • implements an actual fix for the underlying problem
  • unittests the above
  • use the above fix properly, directly within the drivers
    • This is potentially problematic right now, because the QCSchema does not have a field to specify the overlap. I will need to investigate a good solution for this.
    • I settled on adding an additional attribute to the QCSchema which stores the AO overlap. This seems like the best solution at this time.
  • ensure the more complex AngularMomentum is handled properly by any Transformer class
  • add a release note

@coveralls
Copy link

coveralls commented Nov 14, 2023

Pull Request Test Coverage Report for Build 6866355451

  • 79 of 86 (91.86%) changed or added relevant lines in 10 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.008%) to 86.768%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit_nature/second_q/transformers/active_space_transformer.py 11 12 91.67%
qiskit_nature/second_q/transformers/basis_transformer.py 10 12 83.33%
qiskit_nature/second_q/formats/qcschema_translator.py 22 26 84.62%
Files with Coverage Reduction New Missed Lines %
qiskit_nature/second_q/drivers/psi4d/psi4driver.py 1 85.12%
Totals Coverage Status
Change from base Build 6860424299: 0.008%
Covered Lines: 8767
Relevant Lines: 10104

💛 - Coveralls

@mrossinek mrossinek marked this pull request as ready for review November 14, 2023 14:21
Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>
@woodsp-ibm woodsp-ibm added the stable backport potential The bug might be minimal and/or import enough to be port to stable label Nov 14, 2023
@mrossinek
Copy link
Member Author

Do we really want to backport this? It is a significant bug, but this also changes the API of e.g. the AngularMomentum..

@woodsp-ibm
Copy link
Member

The change is backward compatible though with any existing code is it not - the new parameter is optional and it will still do what it did before no? But technically it is an API change, though needed to sort the bug.

@mrossinek mrossinek merged commit 0c0b3c8 into qiskit-community:main Nov 15, 2023
15 checks passed
@mrossinek mrossinek deleted the spin-square-overlap branch November 15, 2023 06:59
mergify bot pushed a commit that referenced this pull request Nov 15, 2023
* feat: include the overlap in the AngularMomentum

Fixes #1273

* test: add unittests for spin operators with overlap

* Add reno

* Update docs

* Add spelling exceptions

* Add missing import

* Handle overlap in AngularMomentum during transformers

* Handle overlap during drivers

* Mention new method to extract overlap from QCSchema in reno

* Update releasenotes/notes/fix-angular-momentum-73eca0a5afb70679.yaml

Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>

---------

Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>
(cherry picked from commit 0c0b3c8)
mergify bot added a commit that referenced this pull request Nov 15, 2023
* feat: include the overlap in the AngularMomentum

Fixes #1273

* test: add unittests for spin operators with overlap

* Add reno

* Update docs

* Add spelling exceptions

* Add missing import

* Handle overlap in AngularMomentum during transformers

* Handle overlap during drivers

* Mention new method to extract overlap from QCSchema in reno

* Update releasenotes/notes/fix-angular-momentum-73eca0a5afb70679.yaml

Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>

---------

Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>
(cherry picked from commit 0c0b3c8)

Co-authored-by: Max Rossmannek <oss@zurich.ibm.com>
ialsina pushed a commit to ialsina/qiskit-nature that referenced this pull request Nov 17, 2023
* feat: include the overlap in the AngularMomentum

Fixes qiskit-community#1273

* test: add unittests for spin operators with overlap

* Add reno

* Update docs

* Add spelling exceptions

* Add missing import

* Handle overlap in AngularMomentum during transformers

* Handle overlap during drivers

* Mention new method to extract overlap from QCSchema in reno

* Update releasenotes/notes/fix-angular-momentum-73eca0a5afb70679.yaml

Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>

---------

Co-authored-by: Steve Wood <40241007+woodsp-ibm@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stable backport potential The bug might be minimal and/or import enough to be port to stable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The AngularMomentum operator is unable to account for spin contamination
3 participants