Separate the physical model into a Model class #104

BoxiLi · 2021-10-22T15:31:31Z

Previously, the Hamiltonian model in Processor is saved as partially initialized pulses. This PR separate the Hamiltonian model from pulse and the Processor class. It can now be defined independently and includes only the information of the Hamiltonian. The pulses will only be generated after the circuit is loaded.

The main changes are in processor.py. In particular in the initialization. The others are either docs change or internal updates to fit this new structure. A bunch of getter functions are added to support the old way of building the Hamiltonians using the Processor. The data is, however, always saved in Model.

In principle ModelProcessor can be merged into Processor, but it is best to wait for another PR.

There are a few FIXME that are used to remind me to revisit some details later in a different PR. They are unrelated to the Hamiltonian model.

@nathanshammah @quantshah

Issue #98

TODO

Add more tests
Check compatibility with existing notebooks.

Breaking changes

The pulse will not be generated before loading the circuit. So inquiring the Hamiltonian operator through pulse will only return an empty list. One should now use e.g. Processor.get_control("sx0") for this.
Processor.get_operators_labels() is renamed as get_latex_str for clearity.

Previously, the Hamiltonian model in `Processor` is saved as partially initialized pulses. This commit separates the Hamiltonian model from pulse and the `Processor` class. It can now be defined independently and includes only the information of the Hamiltonian. The pulses will only be generated after the circuit is loaded. Breaking changes: - processor.pulses will no longer be created while defining the Hamiltonian model. This means that using it to check the available Hamiltonians before loading the circuit will fail. A proper warning will be added in a later commit. Instead, one should use set_coeffs and set_tlist to generate pulses first. Also, because in Model, a dictionary is used to store the Hamiltonian, the order of the generated pulse will also be different than before. - replace get_operator_labels by get_latex_str. This is just to make the name clearer. A proper warning will be added in later commits.

Rewrite the initialization in subclasses to incorporate the new model class. In each subclass of Processor, a model is first created and passed to the initializer of the Processor class.

The key word argument start was added from python 3.8+

Model parameters should be defined while initialization and should not be updated in Processor. This is because updating will not trigger the _compute_params method in the model, making the parameters outdated or incomplete.

src/qutip_qip/device/cavityqed.py

nathanshammah

I left some comments and suggestions.

Actually the code climate issues seem reasonable and could be actioned upon: besides blank spaces, there is a chunk of code involving pulses that maybe could be refactored in a private function.

Great to see these additions and the thorough testing.

nathanshammah · 2021-11-09T12:26:08Z

doc/source/qip-processor.rst

@@ -159,19 +159,19 @@ To let it find the optimal pulses, we need to give the parameters for :func:`~qu


      # Same parameter for all the gates
-      qc = QubitCircuit(N=1)
+      qc = QubitCircuit(1)


Suggested change

qc = QubitCircuit(1)

qc = QubitCircuit(1)

just a question, why did you delete "N=" here and elsewhere?

I'm trying to discourage the use of capital N as the number of qubits, and eventually replace it with num_qubits, to be consistent.

But maybe this should not be included here in this PR, I agree. I can revert it if desired.

nathanshammah · 2021-11-09T12:28:05Z

src/qutip_qip/device/cavityqed.py

+
+class CavityQEDModel(Model):
+    """
+    The physical model for a dispersive cavity-QED system.


Suggested change

The physical model for a dispersive cavity-QED system.

The physical model for a dispersive cavity-QED system.

shall we change "system" with "processor"? Is there any other detail to add?

Let's add brief details about the model Hamiltonian (should be the Tavis-Cummings Hamiltonian).

src/qutip_qip/device/cavityqed.py

src/qutip_qip/device/circuitqed.py

nathanshammah · 2021-11-09T12:36:44Z

src/qutip_qip/device/processor.py

+    def get_noise(self) -> List[Noise]:
+        """
+        Get a list of :obj:`.Noise` objects.
+        Single qubit relaxation (T1, T2) are not included here.


Why are the local T1 and T2 not included here?

Yeah there is a trade-off. They are not included here because they are defined just as two numbers and will be handled in the processor. I can add them directly here but then whoever implements their own Model will need to do it themselves.

On the other hand, separating them is indeed a bit strange. Now looking at them again, I think it might be better to include also T1 T2 here. Whoever implements a custom model will need to do their own homework, which should not be that difficult if they are already at this stage.

BoxiLi · 2021-11-09T12:57:39Z

@nathanshammah

there is a chunk of code involving pulses that maybe could be refactored in a private function.

Which part do you mean?

nathanshammah · 2021-11-16T15:15:06Z

Is this ready for review? The part of code I refer to is the one picked by code climate,

  if isinstance(coeffs, dict):
            pulse_dict = self.get_pulse_dict()
            for pulse_name, value in coeffs.items():
                if pulse_name in pulse_dict:
                    self.pulses[pulse_dict[pulse_name]].coeff = value

anyways, this is fixed now.

BoxiLi · 2021-11-16T15:59:24Z

Is this ready for review?

Yes.

I think it is better to leave a more thorough doc including math equations in a separate PR and get this merged first. We can also update them after the resubmission.

nathanshammah

LGTM, thanks @BoxiLi.

BoxiLi force-pushed the model-class branch 2 times, most recently from eb1e853 to 8db0a2a Compare October 24, 2021 17:58

BoxiLi added 6 commits October 25, 2021 15:04

Unify the initialization

c007e03

Rewrite the initialization in subclasses to incorporate the new model class. In each subclass of Processor, a model is first created and passed to the initializer of the Processor class.

Add the new model class to doc

a1dc578

Correct a wrong doc string in noise

30f41e6

Fix some bugs and typos

ba1d654

Add tests for the new model class

0f96460

BoxiLi force-pushed the model-class branch from 8db0a2a to 0f96460 Compare October 25, 2021 14:17

BoxiLi added 5 commits October 25, 2021 16:27

Minor correction on type hints

52cc852

Change get_latex_str to get_control_latex

dc6b2b6

Correct a bug for optimal control processor with no drift

0615322

Add a test for OptPulseProcessor with model

2cc78de

Add doc: get_control_labels can be optional

113c0f4

BoxiLi force-pushed the model-class branch from efdd4ef to 75a70cc Compare November 1, 2021 16:32

BoxiLi added 2 commits November 3, 2021 12:59

Fix styling and codeclimate issues

c74e70f

Recover python 3.6 support

08f3f27

The key word argument start was added from python 3.8+

BoxiLi force-pushed the model-class branch 3 times, most recently from 2f1617d to eb153e6 Compare November 9, 2021 11:44

BoxiLi added 2 commits November 9, 2021 12:46

Update a few place in the pulse paper

fd7a708

Fix a bug: Model parameters provided to the processor class

8e839d9

Model parameters should be defined while initialization and should not be updated in Processor. This is because updating will not trigger the _compute_params method in the model, making the parameters outdated or incomplete.

BoxiLi force-pushed the model-class branch from eb153e6 to 8e839d9 Compare November 9, 2021 11:46

quantshah reviewed Nov 9, 2021

View reviewed changes

src/qutip_qip/device/cavityqed.py Outdated Show resolved Hide resolved

nathanshammah requested changes Nov 9, 2021

View reviewed changes

Fix typos in docstrings

2488892

BoxiLi force-pushed the model-class branch from 416e7a2 to 2488892 Compare November 9, 2021 22:13

BoxiLi added this to the qutip-qip-0.2.0 milestone Nov 13, 2021

BoxiLi added 2 commits November 16, 2021 17:01

Minor fix in the doc

4bcdd3d

Minor improvement in the doc

2c153d2

BoxiLi requested a review from nathanshammah November 16, 2021 16:06

nathanshammah approved these changes Nov 19, 2021

View reviewed changes

nathanshammah merged commit 995f8e3 into qutip:master Nov 19, 2021

BoxiLi deleted the model-class branch November 25, 2021 08:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separate the physical model into a Model class #104

Separate the physical model into a Model class #104

BoxiLi commented Oct 22, 2021 •

edited

Loading

nathanshammah left a comment

nathanshammah Nov 9, 2021

BoxiLi Nov 9, 2021

nathanshammah Nov 9, 2021

nathanshammah Nov 9, 2021

nathanshammah Nov 9, 2021

BoxiLi Nov 9, 2021

BoxiLi commented Nov 9, 2021

nathanshammah commented Nov 16, 2021

BoxiLi commented Nov 16, 2021

nathanshammah left a comment

	The physical model for a dispersive cavity-QED system.
	The physical model for a dispersive cavity-QED system.

Separate the physical model into a Model class #104

Separate the physical model into a Model class #104

Conversation

BoxiLi commented Oct 22, 2021 • edited Loading

nathanshammah left a comment

Choose a reason for hiding this comment

nathanshammah Nov 9, 2021

Choose a reason for hiding this comment

BoxiLi Nov 9, 2021

Choose a reason for hiding this comment

nathanshammah Nov 9, 2021

Choose a reason for hiding this comment

nathanshammah Nov 9, 2021

Choose a reason for hiding this comment

nathanshammah Nov 9, 2021

Choose a reason for hiding this comment

BoxiLi Nov 9, 2021

Choose a reason for hiding this comment

BoxiLi commented Nov 9, 2021

nathanshammah commented Nov 16, 2021

BoxiLi commented Nov 16, 2021

nathanshammah left a comment

Choose a reason for hiding this comment

BoxiLi commented Oct 22, 2021 •

edited

Loading