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

Electronic Structure Stack Refactoring - Part 2 #841

Merged
merged 15 commits into from
Oct 6, 2022
Merged

Electronic Structure Stack Refactoring - Part 2 #841

merged 15 commits into from
Oct 6, 2022

Conversation

mrossinek
Copy link
Member

@mrossinek mrossinek commented Sep 23, 2022 •
edited
Loading

Summary

This is the second part of the major refactoring of the electronic structure stack.
It does the following:

  • implements FermionicOp.from_polynomial_tensor
  • adds the ElectronicIntegrals container based on the PolynomialTensor (a similar design can be used for Low rank representations #654)
  • refactors the ElectronicEnergy and ElectronicDipoleMoment based on the above
  • refactors the ElectronicStructureProblem to be single-basis only
  • updates the electronic transformers to work with the refactored problem class
  • removes the now obsolete IntegralProperty and properties.intergals.ElectronicIntegrals classes (+ subclasses)

Details and comments

  • missing functionality:
  • documentation
    • module/class/method level
    • release note listing the major features (similar to the list above) but see next point (makes more sense to have with the migration guide)
    • I plan to write the migration guide in one big go for the entire 0.5 release
    • explicit example in ElectronicDipoleMoment for nuclear_dipole_moment as constant shift in PolynomialTensor objects
  • new unittests:
    • for ElectronicIntegrals
    • for BasisTransformer

Closes #666
Closes #679

@mrossinek mrossinek added the run_slow PR will run all unit tests decorated as slow label Sep 23, 2022
@mrossinek mrossinek self-assigned this Sep 23, 2022
mrossinek
mrossinek commented Sep 23, 2022
View reviewed changes
test/second_q/hamiltonians/resources/electronic_energy_op.json
{
Copy link
Member Author

Choose a reason for hiding this comment

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

I had to regenerate this file to get the physicist-ordered integrals

test/second_q/problems/test_vibrational_properties_container.py
Comment on lines -38 to +35
container.occupied_modals = ElectronicDipoleMoment() # type: ignore[assignment]
container.occupied_modals = Magnetization(2) # type: ignore[assignment]
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm just using some other non-vibrational property, since the ElectronicDipoleMoment no longer has an empty-init available

@mrossinek mrossinek force-pushed the fermionicop_from_polynomial_tensor branch from 0b6556b to e7217e8 Compare September 23, 2022 17:37
@mrossinek mrossinek force-pushed the fermionicop_from_polynomial_tensor branch 4 times, most recently from 1fed1cc to 4f86013 Compare September 26, 2022 17:56
@mrossinek mrossinek mentioned this pull request Sep 27, 2022
Closed
@mrossinek mrossinek force-pushed the fermionicop_from_polynomial_tensor branch 4 times, most recently from ed1142d to cb09db3 Compare September 28, 2022 09:28
@mrossinek mrossinek mentioned this pull request Sep 28, 2022
Closed
@mrossinek mrossinek added this to the 0.5.0 milestone Sep 28, 2022
@mrossinek mrossinek force-pushed the fermionicop_from_polynomial_tensor branch from cb09db3 to 586e3b8 Compare September 28, 2022 11:38
@mrossinek mrossinek marked this pull request as ready for review September 28, 2022 11:39
woodsp-ibm
woodsp-ibm reviewed Oct 3, 2022
View reviewed changes
@mrossinek mrossinek force-pushed the fermionicop_from_polynomial_tensor branch from 87c6fe5 to 329cd5c Compare October 6, 2022 14:33
@mrossinek mrossinek requested a review from woodsp-ibm October 6, 2022 14:56
@coveralls
Copy link

Pull Request Test Coverage Report for Build 3199013547

  • 621 of 674 (92.14%) changed or added relevant lines in 23 files are covered.
  • 9 unchanged lines in 6 files lost coverage.
  • Overall coverage increased (+0.2%) to 85.779%
Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit_nature/second_q/drivers/gaussiand/gaussiandriver.py 16 17 94.12%
qiskit_nature/second_q/transformers/base_transformer.py 3 4 75.0%
qiskit_nature/second_q/formats/fcidump/fcidump.py 28 30 93.33%
qiskit_nature/second_q/hamiltonians/electronic_energy.py 27 29 93.1%
qiskit_nature/second_q/operators/polynomial_tensor.py 61 63 96.83%
qiskit_nature/second_q/operators/fermionic_op.py 23 26 88.46%
qiskit_nature/second_q/operators/sparse_label_op.py 23 26 88.46%
qiskit_nature/second_q/formats/qcschema_translator.py 64 69 92.75%
qiskit_nature/second_q/transformers/active_space_transformer.py 77 82 93.9%
qiskit_nature/second_q/transformers/basis_transformer.py 56 62 90.32%
Files with Coverage Reduction New Missed Lines %
qiskit_nature/second_q/hamiltonians/hamiltonian.py 1 78.57%
qiskit_nature/second_q/operators/polynomial_tensor.py 1 92.5%
qiskit_nature/second_q/properties/property.py 1 89.47%
qiskit_nature/second_q/transformers/active_space_transformer.py 1 92.57%
qiskit_nature/second_q/operators/sparse_label_op.py 2 88.99%
qiskit_nature/second_q/formats/fcidump/fcidump.py 3 87.18%
Totals Coverage Status
Change from base Build 3197343594: 0.2%
Covered Lines: 17022
Relevant Lines: 19844
💛 - Coveralls

@mrossinek mrossinek merged commit ce3e8a5 into qiskit-community:main Oct 6, 2022
@mrossinek mrossinek deleted the fermionicop_from_polynomial_tensor branch October 6, 2022 18:29
@mrossinek mrossinek mentioned this pull request Oct 7, 2022
Closed
@mrossinek mrossinek mentioned this pull request Dec 7, 2022
Closed
Anthony-Gandon pushed a commit to Anthony-Gandon/qiskit-nature that referenced this pull request May 25, 2023
@mrossinek @woodsp-ibm
Electronic Structure Stack Refactoring - Part 2 (qiskit-community#841)
0837970 
* [WIP] Fermionic.from_polynomial_tensor integration

* Apply suggestions from code review

Co-authored-by: Steve Wood <[email protected]>

* Apply more suggestions from code review

* refactor: remove explicit register_length argument from PolynomialTensor

* refactor: FermionicOp.from_polynomial_tensor implementation

* refactor: revert usage of Number

* refactor: rename ElectronicIntegrals.mixed to .beta_alpha

* fix: validate keys in SparseLabelOp.register_length.setter

* docs: improve constant offset documentation

* refactor: validate ElectronicIntegrals data

* fix: spell

* fix: set comparison behavior

* fix: revert change of DipoleTuple type

* fix: attempty to fix mypy ambiguity

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

@woodsp-ibm woodsp-ibm woodsp-ibm approved these changes

@ElePT ElePT Awaiting requested review from ElePT

@stefan-woerner stefan-woerner Awaiting requested review from stefan-woerner

@manoelmarques manoelmarques Awaiting requested review from manoelmarques

Assignees

@mrossinek mrossinek

Labels
run_slow PR will run all unit tests decorated as slow
Projects
None yet
Milestone
0.5.0
Development

Successfully merging this pull request may close these issues.

Nuclear repulsion energy is tough to find! Implement FermionicOp.from_polynomial_tensor
3 participants
@mrossinek @coveralls @woodsp-ibm