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

[refactor] Towards a non-driver focused future #701

Closed
mrossinek opened this issue Jun 20, 2022 · 7 comments · Fixed by #746 or #940
Closed

[refactor] Towards a non-driver focused future #701

mrossinek opened this issue Jun 20, 2022 · 7 comments · Fixed by #746 or #940
Assignees
Labels
Epic type: feature request New feature or request
Milestone

Comments

@mrossinek
Copy link
Member

mrossinek commented Jun 20, 2022

What should we add?

This Epic outlines the plan for the upcoming restructuring of the entire Qiskit Nature code base.
Separate issues will be created outlining the refactoring plan for each existing module in more
detail. (Some modules/files will not be migrated. See at the bottom of this description.)

Vision

We are undertaking a step away from the existing control flow in which Qiskit Nature uses
drivers to handle the electronic structure integral generation by calling other classical codes.
Instead, the plan is to design a code- and language-agnostic description (implemented with HDF5) of
a problem for Qiskit Nature to solve, which the classical code can generate directly.
In this way, the responsibility over the control flow of a calculation is handed over to the
classical codes rather than remaining within Qiskit Nature.

This has multiple benefits, a major one being the immediate availability of iterative embedding
techniques already implemented in classical codes (e.g. CASSCF). In Qiskit Nature's current design,
only CASCI calculations are possible and an implementation of CASSCF would require a custom driver
class for each classical code, which is not scalable for the future.

Another issue will be opened in the near future outlining this vision in more detail and discussing
the planned HDF5 file structure.

Code Structure

With such a drastic change in design, we are taking this opportunity to restructure the code base.
The following structure roughly outlines the intended new structure:

qiskit_nature/
    second_quantization/
        algorithms/
            excited_state_solvers/
            ground_state_solvers/
            initial_points/
        circuit/
            library/
                ansatzes/
                initial_states/
        operators/
        mappers/
        hamiltonians/
        properties/
        problems/
        transformers/
        drivers/

A more detailed overview including the main classes is given by the following UML:

overview

Workflow

Someone already familiar with Qiskit Nature, should not have much trouble understanding the
relations outlined by the following class diagram:

relations

Together with this pseudo code, this should roughly outline the intended workflow:

from qiskit_nature.second_quantization.algorithms.ground_state_solvers import (
    GroundStateEigensolver,
    VQEUCCFactory,
)
from qiskit_nature.second_quantization.mappers import JordanWignerMapper, QubitConverter
from qiskit_nature.second_quantization.drivers import HDF5Driver

problem = HDF5Driver("problem_description.hdf5").run()

solver = GroundStateEigensolver(
    VQEUCCFactory(),
    QubitConverter(JordanWignerMapper()),
)

result = solver.solve(problem)

More details on the various modules will be provided by the respective issues.

Unaffected modules

  • constants.py
  • deprecation.py
  • exceptions.py
  • hdf5.py
  • logging.py
  • optionals.py
  • runtime
  • settings.py
  • utils
  • version.py
  • VERSION.txt
@kevinsung
Copy link
Contributor

When doing these kinds of refactorings it's good to try as much as possible to preserve the version control history of each file (so git blame is still useful afterwards).

@mrossinek
Copy link
Member Author

When doing these kinds of refactorings it's good to try as much as possible to preserve the version control history of each file (so git blame is still useful afterwards).

I agree, that would be ideal. However, to some degree this might not be possible because we need to actually copy instead of move the code, in order to abide by our deprecation policies.
There are ways to copy files within a git repository and include the history but I'm not sure how feasible this will be on this large scale.

@kevinsung
Copy link
Contributor

However, to some degree this might not be possible because we need to actually copy instead of move the code, in order to abide by our deprecation policies.

Perhaps we could have one commit that moves the code to the new location, followed by another commit that copies it back to the old.

@ElePT
Copy link
Contributor

ElePT commented Jul 4, 2022

The idea behind this comment is to explain the change in direction that you will see in my refactoring PR (#722). As discussed with @mrossinek and @woodsp-ibm, we will change this proposed structure in favor of a flatter and hopefully more intuitive one, where:

  • the operator_factories module will be split into
    • hamiltonians
    • properties
  • we will keep the original drivers module for the time being. In future refactoring steps this could turn into an api module or similar.
  • mappers and transformers will be exposed as their own modules, instead of being nested in operators and problems
  • not all mappers work with all operators. Instead of creating submodules (fermonic, vibrational, spin), the corresponding operator for each mapper will be indicated via type hinting (For example, JordanWignerMapper will inherit from FermionicMapper)
  • similarly, not all transformers work with all problems. Instead of creating submodules (electronic), the corresponding problem/use for each transformer could be indicated via type hinting (inheriting from ElectronicBaseTransformer).

So, some modules will still follow the structure shown above:

qiskit_nature/
    second_q/
        algorithms/
            excited_state_solvers/
            ground_state_solvers/
            initial_points/
        circuit/
            library/
                ansatzes/
                initial_states/

But we will slightly modify the rest of the modules to follow:

qiskit_nature/
    second_q/
        mappers/
            # all mappers here
            JordanWignerMapper
            LinearMapper
            ...
            QubitMapper
            QubitConverter # included with mappers
      
        operators/
             FermionicOp
             SpinOp
             VibrationalOp
         
         hamiltonians/
                QuadraticHamiltonian
                ElectronicEnergy
                VibrationalEnergy
                LatticeModel
                ...
                lattices/ # used to construct lattice models, right place?
                      Line
                      Square
                      ...       
                      
         properties/  # these are properties of the hamiltonians              
                 AngularMomentum
                 DipoleMoment
                 ...
                 OccupiedModals
                 ...
          
          transformers/
                ActiveSpaceTransformer
                BasisTransformer
                FreezeCoreTransformer
           
           problems/
                ElectronicStructureProblem
                ElectronicStructureResult
                LatticeModelProblem
                LatticeModelResult
                VibrationalStructureProblem
                VibrationalStructureResult
            
            drivers/
                # keep legacy drivers as part of the new structure

Did I overlook anything?

@mrossinek
Copy link
Member Author

mrossinek commented Jul 4, 2022

Thanks for the summary, @ElePT! Looks like you got everything right 👍

EDIT: You may want to indicate that ElectronicStructureResult (and similar classes) will become part of the problems module

@mrossinek
Copy link
Member Author

I am reopening this Epic. I faultily linked it to the PR which does the code restructuring, but more upcoming refactorings are related to this.

@mrossinek mrossinek reopened this Jul 19, 2022
mrossinek added a commit to mrossinek/qiskit-nature that referenced this issue Jul 21, 2022
As outlined in qiskit-community#701, Qiskit Nature is working towards a non-driver
focused future. This means, rather than the `BaseProblem` interface
taking in a `BaseDriver` and an optional list of `BaseTransformer`
instances, the problem itself should be a fully standalone description
of the user's problem of interest.
In other words, `BaseProblem` should be the produced result of a
`BaseDriver` and, by extension, `BaseTransformer` instances should
transform problem instances.

This commit works towards such a future by refactoring the code:
- `BaseProblem` is the returned object of `BaseDriver.run`
- `BaseTransformer.transform` has `BaseProblem` as in- and output
- `BaseProblem` takes over the role of `GroupedProperty`
  - in doing so, `Hamiltonian` objects are more clearly separated from
    `Property` objects (further refactoring to come)
mrossinek added a commit to mrossinek/qiskit-nature that referenced this issue Jul 21, 2022
As outlined in qiskit-community#701, Qiskit Nature is working towards a non-driver
focused future. This means, rather than the `BaseProblem` interface
taking in a `BaseDriver` and an optional list of `BaseTransformer`
instances, the problem itself should be a fully standalone description
of the user's problem of interest.
In other words, `BaseProblem` should be the produced result of a
`BaseDriver` and, by extension, `BaseTransformer` instances should
transform problem instances.

This commit works towards such a future by refactoring the code:
- `BaseProblem` is the returned object of `BaseDriver.run`
- `BaseTransformer.transform` has `BaseProblem` as in- and output
- `BaseProblem` takes over the role of `GroupedProperty`
  - in doing so, `Hamiltonian` objects are more clearly separated from
    `Property` objects (further refactoring to come)
mrossinek added a commit to mrossinek/qiskit-nature that referenced this issue Jul 21, 2022
As outlined in qiskit-community#701, Qiskit Nature is working towards a non-driver
focused future. This means, rather than the `BaseProblem` interface
taking in a `BaseDriver` and an optional list of `BaseTransformer`
instances, the problem itself should be a fully standalone description
of the user's problem of interest.
In other words, `BaseProblem` should be the produced result of a
`BaseDriver` and, by extension, `BaseTransformer` instances should
transform problem instances.

This commit works towards such a future by refactoring the code:
- `BaseProblem` is the returned object of `BaseDriver.run`
- `BaseTransformer.transform` has `BaseProblem` as in- and output
- `BaseProblem` takes over the role of `GroupedProperty`
  - in doing so, `Hamiltonian` objects are more clearly separated from
    `Property` objects (further refactoring to come)
@mrossinek
Copy link
Member Author

Minor update: I updated the issue description, including the UML graphics, to reflect the design changes as also discussed by @ElePT further up

mrossinek added a commit to mrossinek/qiskit-nature that referenced this issue Jul 21, 2022
As outlined in qiskit-community#701, Qiskit Nature is working towards a non-driver
focused future. This means, rather than the `BaseProblem` interface
taking in a `BaseDriver` and an optional list of `BaseTransformer`
instances, the problem itself should be a fully standalone description
of the user's problem of interest.
In other words, `BaseProblem` should be the produced result of a
`BaseDriver` and, by extension, `BaseTransformer` instances should
transform problem instances.

This commit works towards such a future by refactoring the code:
- `BaseProblem` is the returned object of `BaseDriver.run`
- `BaseTransformer.transform` has `BaseProblem` as in- and output
- `BaseProblem` takes over the role of `GroupedProperty`
  - in doing so, `Hamiltonian` objects are more clearly separated from
    `Property` objects (further refactoring to come)
mrossinek added a commit to mrossinek/qiskit-nature that referenced this issue Jul 21, 2022
As outlined in qiskit-community#701, Qiskit Nature is working towards a non-driver
focused future. This means, rather than the `BaseProblem` interface
taking in a `BaseDriver` and an optional list of `BaseTransformer`
instances, the problem itself should be a fully standalone description
of the user's problem of interest.
In other words, `BaseProblem` should be the produced result of a
`BaseDriver` and, by extension, `BaseTransformer` instances should
transform problem instances.

This commit works towards such a future by refactoring the code:
- `BaseProblem` is the returned object of `BaseDriver.run`
- `BaseTransformer.transform` has `BaseProblem` as in- and output
- `BaseProblem` takes over the role of `GroupedProperty`
  - in doing so, `Hamiltonian` objects are more clearly separated from
    `Property` objects (further refactoring to come)
mrossinek added a commit to mrossinek/qiskit-nature that referenced this issue Jul 21, 2022
As outlined in qiskit-community#701, Qiskit Nature is working towards a non-driver
focused future. This means, rather than the `BaseProblem` interface
taking in a `BaseDriver` and an optional list of `BaseTransformer`
instances, the problem itself should be a fully standalone description
of the user's problem of interest.
In other words, `BaseProblem` should be the produced result of a
`BaseDriver` and, by extension, `BaseTransformer` instances should
transform problem instances.

This commit works towards such a future by refactoring the code:
- `BaseProblem` is the returned object of `BaseDriver.run`
- `BaseTransformer.transform` has `BaseProblem` as in- and output
- `BaseProblem` takes over the role of `GroupedProperty`
  - in doing so, `Hamiltonian` objects are more clearly separated from
    `Property` objects (further refactoring to come)
mrossinek added a commit to mrossinek/qiskit-nature that referenced this issue Jul 21, 2022
As outlined in qiskit-community#701, Qiskit Nature is working towards a non-driver
focused future. This means, rather than the `BaseProblem` interface
taking in a `BaseDriver` and an optional list of `BaseTransformer`
instances, the problem itself should be a fully standalone description
of the user's problem of interest.
In other words, `BaseProblem` should be the produced result of a
`BaseDriver` and, by extension, `BaseTransformer` instances should
transform problem instances.

This commit works towards such a future by refactoring the code:
- `BaseProblem` is the returned object of `BaseDriver.run`
- `BaseTransformer.transform` has `BaseProblem` as in- and output
- `BaseProblem` takes over the role of `GroupedProperty`
  - in doing so, `Hamiltonian` objects are more clearly separated from
    `Property` objects (further refactoring to come)
mrossinek added a commit to mrossinek/qiskit-nature that referenced this issue Jul 21, 2022
As outlined in qiskit-community#701, Qiskit Nature is working towards a non-driver
focused future. This means, rather than the `BaseProblem` interface
taking in a `BaseDriver` and an optional list of `BaseTransformer`
instances, the problem itself should be a fully standalone description
of the user's problem of interest.
In other words, `BaseProblem` should be the produced result of a
`BaseDriver` and, by extension, `BaseTransformer` instances should
transform problem instances.

This commit works towards such a future by refactoring the code:
- `BaseProblem` is the returned object of `BaseDriver.run`
- `BaseTransformer.transform` has `BaseProblem` as in- and output
- `BaseProblem` takes over the role of `GroupedProperty`
  - in doing so, `Hamiltonian` objects are more clearly separated from
    `Property` objects (further refactoring to come)
mrossinek added a commit to mrossinek/qiskit-nature that referenced this issue Jul 21, 2022
As outlined in qiskit-community#701, Qiskit Nature is working towards a non-driver
focused future. This means, rather than the `BaseProblem` interface
taking in a `BaseDriver` and an optional list of `BaseTransformer`
instances, the problem itself should be a fully standalone description
of the user's problem of interest.
In other words, `BaseProblem` should be the produced result of a
`BaseDriver` and, by extension, `BaseTransformer` instances should
transform problem instances.

This commit works towards such a future by refactoring the code:
- `BaseProblem` is the returned object of `BaseDriver.run`
- `BaseTransformer.transform` has `BaseProblem` as in- and output
- `BaseProblem` takes over the role of `GroupedProperty`
  - in doing so, `Hamiltonian` objects are more clearly separated from
    `Property` objects (further refactoring to come)
mrossinek added a commit to mrossinek/qiskit-nature that referenced this issue Jul 21, 2022
As outlined in qiskit-community#701, Qiskit Nature is working towards a non-driver
focused future. This means, rather than the `BaseProblem` interface
taking in a `BaseDriver` and an optional list of `BaseTransformer`
instances, the problem itself should be a fully standalone description
of the user's problem of interest.
In other words, `BaseProblem` should be the produced result of a
`BaseDriver` and, by extension, `BaseTransformer` instances should
transform problem instances.

This commit works towards such a future by refactoring the code:
- `BaseProblem` is the returned object of `BaseDriver.run`
- `BaseTransformer.transform` has `BaseProblem` as in- and output
- `BaseProblem` takes over the role of `GroupedProperty`
  - in doing so, `Hamiltonian` objects are more clearly separated from
    `Property` objects (further refactoring to come)
@mergify mergify bot closed this as completed in #940 Nov 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Epic type: feature request New feature or request
Projects
None yet
3 participants