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

Towards a non-driver future #796

Merged
merged 40 commits into from
Sep 1, 2022

Conversation

mrossinek
Copy link
Member

@mrossinek mrossinek commented Aug 18, 2022

Summary

This is the major PR which untangles the intertwined interfaces of BaseProblem, BaseDriver and BaseTransformer.
There is no clean way to do this unfortunately, so this PR is rather massive...

Details and comments

I used this opportunity to also do some reshuffling to place the classes into their intended location. Most notably the population of the qiskit_nature.second_q.hamiltonians module.
While this does add a bit of complexity to the PR, this is not a significant addition of the overall complexity and it allowed me to not have to introduce some dummy classes to ensure the API of the new BaseProblem looks correct.

I do not recommend a commit-wise review of this PR due to its convoluted nature. The commits are not atomic and do not represent fully working states at each step of the way. Churning through everything as a bulk PR is advisable, albeit cumbersome.

The following is a non-exhaustive list of todos, before this PR can be merged:

  • iterate all changed files to verify changes
  • write/update documentation
  • add an exhaustive release note

From a technical perspective, this PR should already be ready to review, though.

⚠️ Note, that this PR currently removes more things than the should be the case in the end. Most notably, more drivers are being removed. These should be re-added after this PR, following the examples as done here for PySCF and Gaussian.

This is a first step towards decoupling the BaseProblem and BaseDriver
interfaces. Instead of injecting drivers and transformers into a
problem, these become the output of our driver classes. In turn,
transformers now take entire problem instances as input and return them
as output.

This is an intermediate step towards the final form of the problem
classes and this commit is not working fully.
This moves the "main operators" into the new hamiltonians module and
creates a base interface for these classes to follow.
- extracts a simple MoleculeInfo dataclass out of the original Molecule
- moves UnitsType to DistanceType in the new units module (generalizing
  the concept of enumerating units)
@mrossinek mrossinek added the run_slow PR will run all unit tests decorated as slow label Aug 18, 2022
@mrossinek mrossinek requested a review from woodsp-ibm August 18, 2022 17:39
@mrossinek mrossinek changed the title Towards a non-driver future - Take 2 Towards a non-driver future Aug 18, 2022
@coveralls
Copy link

coveralls commented Aug 18, 2022

Pull Request Test Coverage Report for Build 2973604536

  • 642 of 717 (89.54%) changed or added relevant lines in 42 files are covered.
  • 77 unchanged lines in 11 files lost coverage.
  • Overall coverage decreased (-0.3%) to 85.295%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit_nature/second_q/algorithms/initial_points/hf_initial_point.py 3 4 75.0%
qiskit_nature/second_q/algorithms/initial_points/mp2_initial_point.py 4 5 80.0%
qiskit_nature/second_q/hamiltonians/fermi_hubbard_model.py 6 7 85.71%
qiskit_nature/second_q/hamiltonians/heisenberg_model.py 5 6 83.33%
qiskit_nature/second_q/hamiltonians/ising_model.py 23 24 95.83%
qiskit_nature/second_q/hamiltonians/lattices/lattice.py 22 23 95.65%
qiskit_nature/second_q/hamiltonians/quadratic_hamiltonian.py 4 5 80.0%
qiskit_nature/second_q/problems/vibrational_structure_problem.py 23 24 95.83%
qiskit_nature/second_q/properties/bases/electronic_basis_transform.py 2 3 66.67%
qiskit_nature/second_q/drivers/electronic_structure_driver.py 96 98 97.96%
Files with Coverage Reduction New Missed Lines %
qiskit_nature/second_q/drivers/base_driver.py 1 85.71%
qiskit_nature/second_q/drivers/electronic_structure_driver.py 1 97.32%
qiskit_nature/second_q/drivers/gaussiand/gaussian_forces_driver.py 1 64.71%
qiskit_nature/second_q/drivers/vibrational_structure_driver.py 1 87.5%
qiskit_nature/second_q/transformers/active_space_transformer.py 1 91.79%
qiskit_nature/second_q/properties/property.py 4 81.58%
qiskit_nature/second_q/properties/integrals/integral_property.py 7 82.05%
qiskit_nature/second_q/properties/particle_number.py 7 85.58%
qiskit_nature/second_q/drivers/pyscfd/pyscfdriver.py 14 76.34%
qiskit_nature/second_q/properties/integrals/vibrational_integrals.py 17 72.73%
Totals Coverage Status
Change from base Build 2953353539: -0.3%
Covered Lines: 16693
Relevant Lines: 19571

💛 - Coveralls

woodsp-ibm
woodsp-ibm previously approved these changes Sep 1, 2022
woodsp-ibm
woodsp-ibm previously approved these changes Sep 1, 2022
@mergify mergify bot merged commit 2c292b5 into qiskit-community:main Sep 1, 2022
@mrossinek mrossinek deleted the refactor-problems2 branch September 2, 2022 07:49
Anthony-Gandon pushed a commit to Anthony-Gandon/qiskit-nature that referenced this pull request May 25, 2023
* WIP: problem refactoring

This is a first step towards decoupling the BaseProblem and BaseDriver
interfaces. Instead of injecting drivers and transformers into a
problem, these become the output of our driver classes. In turn,
transformers now take entire problem instances as input and return them
as output.

This is an intermediate step towards the final form of the problem
classes and this commit is not working fully.

* refactor: make BaseProblem class instantiable

* refactor: integrate PropertiesContainer

* refactor: populate second_q.hamiltonians

This moves the "main operators" into the new hamiltonians module and
creates a base interface for these classes to follow.

* refactor: extract lattice constructor methods to Lattice

* refactor: remove unneeded Property subclasses

* fix: some linting

* fix: html generation

* fix: add missing hamiltonian.interpret call

* WIP: migrate driver to QCSchema

* refactor: molecule dataclass

- extracts a simple MoleculeInfo dataclass out of the original Molecule
- moves UnitsType to DistanceType in the new units module (generalizing
  the concept of enumerating units)

* fix: spelling

* fix: PySCFDriver

* fix HDF5Driver-based tests

* fix: GaussianDriver

* fix linters

* refactor transformer tests

* fix: handle optionals in unittests

* fix: GaussianDriver.from_matrix_file

* fix: README example

* fix: tutorials

* fix: slow PySCF-dependent test

* fix: revert minor change to PySCFDriver

* test: fix InitialPoint unittests

* docs: replace open TODOs in docstrings

* add reno

* refactor: use direct type hints for Hamiltonians

* fix: default to ANGSTROM unit in MoleculeInfo

* fix: do not copy LatticeModel.lattice

* fix: generalize MoleculeInfo.coords type

* fix: remove unused loggers

* refactor: enforce keyword arguments in drivers

* refactor: extract _QCSchemaData

* refactor: some QCSchema methods

* fix: update some docstrings

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

* refactor: update argument name in transformers

* refactor: extract np.asarray into reshape utility

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
Labels
automerge run_slow PR will run all unit tests decorated as slow
Projects
None yet
Development

Successfully merging this pull request may close these issues.

refactor: Drivers should produce Problems Support additional data in DriverMetadata
3 participants