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

Implement detecting and transforming index order of two body electronic integrals #520

Merged

Conversation

jlapeyre
Copy link
Contributor

@jlapeyre jlapeyre commented Jan 26, 2022

  • Support AO input matrix in chemists', physicists', "intermediate", or unknown index-order convention. Convert or raise an error for unsupported input.
  • Tests to cover changes.
  • Updated the documentation accordingly.
  • Release note.
  • I have read the CONTRIBUTING document.

Summary

When constructing TwoBodyElectronicIntegrals validate the data and possibly correct it.

Closes #337

Note, we considered the following, but they will not be included here:
Support spin-orbital input matrices.
Include changing spin-orbital index convention? Maybe a subsequent PR

Details and comments

This PR will transform physicists' index-order convention to chemists'. It will also raise an error on detecting an unsupported format or an unrecognized format.

@mrossinek
Copy link
Member

Thanks @jlapeyre, this looks like a very good start! I only skimmed through the code just now so no in detail comments yet. Just one question: I find the choice for the filename starting with _ a bit odd. Maybe we can find a more descriptive name? Since it's already inside of a utils folder I don't think we need the _utils at the end either.
What about two_body_symmetry_conversion.py? Or something along those lines?

@coveralls
Copy link

coveralls commented May 13, 2022

Pull Request Test Coverage Report for Build 3061967887

  • 65 of 66 (98.48%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.04%) to 85.504%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit_nature/second_q/operators/tensor_ordering.py 65 66 98.48%
Totals Coverage Status
Change from base Build 3054069499: 0.04%
Covered Lines: 17118
Relevant Lines: 20020

💛 - Coveralls

@mrossinek mrossinek marked this pull request as draft June 15, 2022 13:11
@mrossinek mrossinek removed the request for review from pbark June 29, 2022 15:27
@jlapeyre
Copy link
Contributor Author

The changes made by @javabster all look great! This PR looks good to go to me.

@jlapeyre jlapeyre changed the title [WIP] Implement detecting and transforming index order of two body electronic integrals Implement detecting and transforming index order of two body electronic integrals Aug 19, 2022
@jlapeyre jlapeyre marked this pull request as ready for review August 19, 2022 14:22
@mrossinek
Copy link
Member

The reason that this has not progressed further, is that I am awaiting the implementation of the PolynomialTensor (#756) and SparseLabelOp (#731) after which we can tackle #666 which is going to largely refactor the existing FermionicOp. Once this happens, we will always store the electronic integrals in physicist order, at which point these conversion utilities will come in very handy to simplify the construction of said integral containers.

I envision these utilities to be exposed very publicly to simplify the construction of PolynomialTensor instances which store electronic integrals. I am potentially even considering an electronic-dedicated subclass of this tensor class, which can handle these integral conversions as well as deal with symmetry reductions/expansions to/from 8-fold symmetry in the future.

Long story short, this will get unblocked once the above issues are resolved, at which point a good location for these utilities needs to be found in the new stack 👍

@mrossinek mrossinek marked this pull request as draft August 22, 2022 19:45
@mrossinek mrossinek marked this pull request as ready for review September 15, 2022 11:02
@mrossinek
Copy link
Member

I have updated this PR and moved the symmetry tools to qiskit_nature.second_q.operators.utils where it can live for now. I might change things a bit more when working on #666 which is next up on my todo list. When doing that and updating the electronic-structure related hamiltonians and properties to use that method, I will be making use of these tools for easier construction of the PolynomialTensor objects.

@mrossinek mrossinek removed the on hold Can not fix yet label Sep 15, 2022
@mrossinek mrossinek force-pushed the two-body-integral-index-transform branch from 4923c1c to f55155d Compare September 15, 2022 14:42
@mrossinek mrossinek force-pushed the two-body-integral-index-transform branch from e3d74d9 to 373fe1d Compare September 15, 2022 16:06
@mergify mergify bot merged commit b88f617 into qiskit-community:main Sep 15, 2022
Anthony-Gandon pushed a commit to Anthony-Gandon/qiskit-nature that referenced this pull request May 25, 2023
…ic integrals (qiskit-community#520)

* Implement detecting and transforming index order of two body electronic integrals

* Fixed typos

* Rename utils file and first pass at enum refaactor

* ✨ lint ✨ and reno

* Added tests

* ✨ lint! ✨

* ✨ lint again! ✨

* Updated test constants

* Remove comments

* Added functions and tests for converting intermediates

* ✨ Lint ✨

* Add error handling for unknown index

* Fix lint error

* Added unknown index test

* converted tests to ddt

* Migrate code to new second_q stack

* docs: update naming, type hints and docstrings

* refactor: move module

Co-authored-by: Abby Mitchell <[email protected]>
Co-authored-by: Abby Mitchell <[email protected]>
Co-authored-by: Max Rossmannek <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add TwoBodyElectronicIntegrals notation conversion utilities
6 participants