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

Adds single qubit decomposition functions #67

Merged
merged 25 commits into from
Jul 19, 2021

Conversation

purva-thakre
Copy link
Contributor

@purva-thakre purva-thakre commented Jun 28, 2021

Adds general single qubit decomposition functions.

  • There's only 2 combinations of decompose_to_rotation_matrices because the calculations for others need to be re-calculated. The outputs were not agreeing with input gates. Will come back to these after a general method for n qubits has been added.
  • Same as above, there's only 1 combinations for ABC_decomposition and others will be added towards the end.
  • The issue with extract_global_phase function is still giving an output off by a phase. In the example notebook, all the outputs are the same as input except for sigmay. As I was able to use average_gate_fidelity to compare the outputs even when they are off by a phase, I plan to come back to this towards the end. Too many days were spent trying to fix this.

Initially, most of the outputs except for sigmay` were off by a phase_factor of -1. The problem was solved by changing the global phase angle.

decomposition_pauliy_off_

Imports, style etc will be corrected after final approval.

Fixes #73

@purva-thakre purva-thakre marked this pull request as draft June 28, 2021 10:14
@purva-thakre purva-thakre marked this pull request as ready for review July 5, 2021 04:39
Comment on lines 94 to 96
_rotation_matrices_dictionary ={"ZYZ": _ZYZ_rotation,
"ZXZ": _ZXZ_rotation,
}
Copy link
Member

@BoxiLi BoxiLi Jul 5, 2021

Choose a reason for hiding this comment

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

You can use the relation X=HZH and Z=HXH, where H is the Hadamard transformation, to generalize to other cases. Because of Y=-HYH, you can get "XYX" and "XZX" decomposition. I use here just Pauli matrices but the rotation is the same because RX(t)=exp(-iXt) (maybe a 1/2 is missing). The transformation changes the rotation axes.

Now I think more about it, if we want, we can also find unitaries that generalize this to other cases. You can understand a Hadamard as roughly a rotation over the Y axis, so it changes RX to RZ. Therefore, one should be able to find a 90-degree rotation over X or Z that does a similar job, e.g. changes RY to RZ.

Copy link
Member

Choose a reason for hiding this comment

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

The routine could be like this (just my thought, not that you have to):

  • Regardless of the scheme, always use ZYZ decomposition, get the 3 angles and the global phase.
  • For each scheme, there is a private function to decide what is the 3 rotation gates to use (RZ, RY or RZ) as well as the global phase correction. They always use the same rotation angles calculated for ZYZ (maybe add a minus sign here and there if needed). This can be manually derived as I sketched above.

In this way one only need one function to calculate the ZYZ angles, the rest is just tranformation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's what I attempted to do by creating _angles_for_ZYZ.

I had calculated the angles for the new combinations by using output of ZYZ and transforming Rz as product of RxRyRx and so on.. I made a mistake somewhere in these transformations and the outputs don't match.

Adding the combinations later down the line is just a matter of re-checking these calculations.

@BoxiLi BoxiLi self-requested a review July 6, 2021 11:45
Copy link
Member

@BoxiLi BoxiLi left a comment

Choose a reason for hiding this comment

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

I went through the code. And left a few comments. Generally looks good! Do you still plan to add schemes to this like ZXZ etc? Or do you prefer to merge this first?

src/qutip_qip/decompositions/decompositions_extras.py Outdated Show resolved Hide resolved
src/qutip_qip/decompositions/__init__.py Outdated Show resolved Hide resolved
src/qutip_qip/decompositions/decompositions_extras.py Outdated Show resolved Hide resolved
src/qutip_qip/decompositions/decompositions_extras.py Outdated Show resolved Hide resolved
src/qutip_qip/decompositions/decompositions_extras.py Outdated Show resolved Hide resolved
src/qutip_qip/decompositions/general_decompositions.py Outdated Show resolved Hide resolved
src/qutip_qip/decompositions/general_decompositions.py Outdated Show resolved Hide resolved
src/qutip_qip/decompositions/single_decompositions.py Outdated Show resolved Hide resolved
@purva-thakre
Copy link
Contributor Author

purva-thakre commented Jul 9, 2021

@hodgestar @BoxiLi @nathanshammah

Going back to this Tuesday's discussion of renaming decompositions to decompose - what's your opinion of renaming single_decompositions to single_qubit_decompose or decompose_single_qubit_gate ?

@BoxiLi
Copy link
Member

BoxiLi commented Jul 11, 2021

decompose_single_qubit_gate sounds nice to me. We could have unified names for this module:

  • decompose_single_qubit_gate(gate, method, ...)
  • decompose_two_qubit_gate(gate, method, ...)
  • decompose_gate(gate, method, ...)

@purva-thakre
Copy link
Contributor Author

purva-thakre commented Jul 12, 2021

decompose_single_qubit_gate sounds nice to me. We could have unified names for this module:

  • decompose_single_qubit_gate(gate, method, ...)
  • decompose_two_qubit_gate(gate, method, ...)
  • decompose_gate(gate, method, ...)

I think this seems like a good idea. Define a dictionary for all the methods specific to the number of qubits.
Would you prefer to make functions like decompose_to_rotation_matrices and ABC_Decomposition be made private ?

This could also be where optimization methods are called. The possibilities right now - least number of gates, least number of CNOT gates.

@purva-thakre
Copy link
Contributor Author

purva-thakre commented Jul 15, 2021

@BoxiLi @hodgestar

Changed decomposition functions being a subpackage to a submodule. All other functions in the subpackage have been made private with only from qutip_qip.decompose import decompose_one_qubit_gate publicly available.

The functions decompose_to_rotation_matrices and ABC_decomposition can be called via strings in decompose_one_qubit_gate. If it's preferred to keep the functions separate, I will undo last commit.

image

Here's an example screenshot :

image

@BoxiLi
Copy link
Member

BoxiLi commented Jul 16, 2021

Changed decomposition functions being a subpackage to a submodule.

Maybe you two discussed this already, but what is the motivation behind this change?

It doesn't (shouldn't) make a difference in the user interface I think because we only expose those public APIs in decompose\__init__.py. However, now we have a separate private subpackage and a module that calls that package. Seems a bit confusing to someone looking at the source code.

@purva-thakre
Copy link
Contributor Author

purva-thakre commented Jul 16, 2021

@BoxiLi We discussed some style changes as well as removing target and num_qubits as input to single qubit functions. The targets of these outputs will be changed from gray code ordering function.

but what is the motivation behind this change?

I wanted to use an easier way to import the main decomposition functions if they were all in one file.

Before yesterday's changes, if there was a need to import some function it would be like below :

from qutip_qip.decompose.some_decomposition_functions_folder_name import decompose_n_qubit gate 

After creating decompose.py outside the folder containing functions defined for decomposing the gates, the import changes as

from qutip_qip.decompose import decompose_n_qubit gate 

I could keep all of these in the same folder if you would prefer the import path be changed in __init__.py via pythonpath. I was unsure if this was a good idea.

@BoxiLi
Copy link
Member

BoxiLi commented Jul 16, 2021

Ok, I think this is the key point here:

Before yesterday's changes, if there was a need to import some function it would be like below :
from qutip_qip.decompose.some_decomposition_functions_folder_name import decompose_n_qubit gate

If I understand correctly, you were saying that if we use a subpackage structure like this:

- qutip_qip
    - decompose
        - __init__.py
        - file.py

And there is a function decompose_single_qubit_gate defined in the file file.py.
The user can only use decompose_single_qubit_gate by

from qutip_qip.decompose.file import `decompose_single_qubit_gate`

Is that correct?

I don't think the above is true because if you import decompose_single_qubit_gate in __init__.py, you can already use

from qutip_qip.decompose import decompose_single_qubit_gate

That is what happening in e.g. devices subpackage.

Or did I miss understand you?

@purva-thakre
Copy link
Contributor Author

Or did I miss understand you?

Not really. There was an issue with my local system path which could only import things as long as the entire path was specified exactly. I will switch back to what it was before.

Thanks !

Alt Text

@purva-thakre
Copy link
Contributor Author

@hodgestar @BoxiLi

I have pushed the new discussed changes.

Looking back, I don't think we discussed decomposed_gates_to_circuit and compute_unitary. These functions were defined for convenience. Anyone can also use already defined things in qip to create a circuit and calculate the matrix of the gates. Let me know if you prefer to discard these functions and show the steps to take for both in a tutorial.

The two functions are very general and can be promoted as class method, rather than being restricted in the decomposition module.
@BoxiLi
Copy link
Member

BoxiLi commented Jul 18, 2021

Looking back, I don't think we discussed decomposed_gates_to_circuit and compute_unitary. These functions were defined for convenience. Anyone can also use already defined things in qip to create a circuit and calculate the matrix of the gates. Let me know if you prefer to discard these functions and show the steps to take for both in a tutorial.

They are indeed useful functions, we can surely keep them in the code. However, we can make them a bit more general:

  • The name decomposed_gates_to_circuit is appropriate in the application here indeed, but the function itself is much more general than this. So we should use a more general name.
  • The two functions are closely related to QubitCircuit. Following the convention of QubitCircuit, we can add them as class methods. This will also save us some input checks.

I made these two minor changes in a commit, feel free to criticize.

@purva-thakre
Copy link
Contributor Author

@BoxiLi Looks good to me !

I could have also made the changes if you would have replied to an earlier comment ! Thanks ! 😸

@BoxiLi
Copy link
Member

BoxiLi commented Jul 18, 2021

I could have also made the changes if you would have replied to an earlier comment ! Thanks ! 😸

Yeah, I definitely believe that you would have made the same changes. I missed that question so failed to reply on time. The PR is in good shape to me. I was considering that we could merge it on Monday if @hodgestar has no further suggestions.

Co-authored-by: Boxi Li <etamin1201@gmail.com>
Copy link
Contributor

@hodgestar hodgestar left a comment

Choose a reason for hiding this comment

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

Looking good to me!

src/qutip_qip/circuit.py Outdated Show resolved Hide resolved
src/qutip_qip/circuit.py Show resolved Hide resolved
@purva-thakre purva-thakre requested a review from BoxiLi July 19, 2021 15:55
@BoxiLi BoxiLi merged commit 16818cc into qutip:master Jul 19, 2021
@BoxiLi
Copy link
Member

BoxiLi commented Jul 19, 2021

Good job! You can now rebase your changes for two qubits gate decomposition on this. @purva-thakre

@purva-thakre purva-thakre deleted the single_qubit_functions branch July 19, 2021 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Understand issue with global phase factor function
3 participants