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

QMolecule Design #148

Closed
pbark opened this issue Apr 21, 2021 · 2 comments · Fixed by #303
Closed

QMolecule Design #148

pbark opened this issue Apr 21, 2021 · 2 comments · Fixed by #303

Comments

@pbark
Copy link
Contributor

pbark commented Apr 21, 2021

Disclaimer: All of the following also applies to the VibrationalStructureProblem and WatsonHamiltonian. However, due to my personal expertise I will give examples almost exclusively in the electronic-framework.

The QMolecule object has (and most likely will) remain unchanged for the past releases except for a few hot-patch additions of some attributes. It serves the purpose of a unified result object of all FermionicDriver implementations.
However, Qiskit Nature's remaining code base has matured a lot and has become a very modular-oriented framework, especially since the introduction of the SecondQuantizedOp in the last release.

Right now, we can observe a lot of "utility"-like methods appearing in multiple locations of the code. For example:

This situation could be improved by a modular design of properties or observables which should be evaluated during the solution of a problem. The following UML shows an example:
Qiskit_Nature-Modular_QMolecule
With such modular objects, we could gather all of the property-specific code in a single, logical location from where it can be used by the rest of the stack. This includes:

class Basis(Enum):
   AO = "ao"
   MO = "mo"

class Property:

   name: str
   """To be used as the aux_op name."""

   basis: Basis
   """Most likely we only ever need the MO basis in the later parts of the stack."""

   register_length: int
   """Essentially the number of modes but makes no naming differentiation between
   electronic, vibrational, etc. use cases."""

   def second_q_ops(self):  # may be named operators instead, in view of a first_quantization future
      # replaces the builder methods from the problems module

   def _integrals(self):
      # replaces the integral_calculators

   def interpret(self, raw_result):
      # WIP: may simplify result-parsing logic

Because such a change is rather intrusive into the existing framework, I propose to do the development in the following three steps.

1. Transformer-independent implementation

We start out with an implementation which is independent of the qiskit_nature.transformers module. This means, that the modular containers will be constructed out of the finally transformed QMolecule before being passed on to the remaining stack.
As an example, below is the adapted ElectronicStructureProblem.second_q_ops method:

    def second_q_ops(self) -> Dict[str, SecondQuantizedOp]:
        self._molecule_data = cast(QMolecule, self.driver.run())
        self._molecule_data_transformed = cast(QMolecule, self._transform(self._molecule_data))

        # construct properties out of self._molecule_data_transformed
        properties: List[Property] = ...

        second_quantized_ops = {prop.name: prop.second_q_ops() for prop in properties}

        return second_quantized_ops

2. Investigate possible Transformer-refactoring

Once we have an initial implementation of the above working, we should investigate the possibility of refactoring the qiskit_nature.transformers module. The questions which need to be answered here include the following:

  • Would a transformer-implementation benefit from a more modular input?
  • Would a transformer require information spread across multiple properties?
    • I can already say, that the ActiveSpaceTransformer will need the ParticleNumber information besides the actual property to be transformed (i.e. ElectronicEnergy or DipoleMoment). In view of a future application such as [Feature] HF/DFT Embedding Schemes #26 it will also require the ElectronicDensity property when working with a non-diagonal density matrix.

3. Investigation of QMolecule as the driver output

Currently, the drivers output a QMolecule or WatsonHamiltonian based on the driver type. While we do not initially plan on changing this, if we reach the end of the previous section, we will have gained more insight and the implementation of the properties may show that we could benefit from a modular driver output.
I do not suggest that this is a necessary step to take, but we should at least re-evaluate this possibility once we get this far in the development.

@mrossinek
Copy link
Member

With the updated description of this issue above, I propose to convert this issue into an Epic after everyone had time to review it. I will create separate implementation-oriented issues for the three mentioned subtasks which can then be assigned to sprints in order to gradually close this (then) Epic.

I would like to stress the benefit of the proposed step-by-step implementation again, because it will allow us to start working on a prototype for the containers while working on #147 in parallel. If we get towards the end of #147 before reaching the third proposed subtask of this issue we will also have gained a better overview of the drivers themselves which will help in the discussion of step 3 once we get to it.

@mrossinek
Copy link
Member

As part of #173 we discussed the possibility to further split qiskit_nature.drivers.second_quantization into:

qiskit_nature/
    drivers/
        second_quantization/
            electronic/
                <...>
            vibrational/
                <...>

As such, renaming QMolecule -> ElectronicDriverResult and WatsonHamiltonian -> VibrationalDriverResult would come very naturally. However, we decided to postpone this until we have a clearer picture of how the implementation of this issue will affect the drivers. Thus, this should be revisited as part of Step 3. in the original issue description.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants