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

Added Rxx and Ryy to gateset #194

Merged
merged 2 commits into from
Mar 22, 2023
Merged

Added Rxx and Ryy to gateset #194

merged 2 commits into from
Mar 22, 2023

Conversation

ajrazander
Copy link
Contributor

Added Rxx and Ryy two-qubit gates to the qutip-qip's gateset. Both of these are native gates for trapped ions (just a matter of laser phase).

Still to do/discuss:
[] The Rxx gate is the same as the current molmer_sorensen gate. Do we keep both or only one?
[] What's the process for adding tests for these two gates?

@BoxiLi
Copy link
Member

BoxiLi commented Mar 5, 2023

Thank you!

The Rxx gate is the same as the current molmer_sorensen gate. Do we keep both or only one?

This is a good question. We should reduce the redundancy as much as possible. Keeping both of them will be very confusing for others.

Now thinking about the similarity between MS gate and RXX/RYY. Do you think it makes sense to put two of them under the name molmer_sorensen? We could add an additional argument to it
molmer_sorensen(theta, phase="x", N=None, targets=[0, 1])
and return the corresponding matrix.

Maybe we could also make the phase more general as a complex number? But I don't know if that has a physical meaning.

What's the process for adding tests for these two gates?

What you have added so far are just functions that return the numpy array. To make them accessible through gates, you also need to add the gate class and make this gate available in a few other places. Like in https://github.com/qutip/qutip-qip/pull/193/files

@BoxiLi
Copy link
Member

BoxiLi commented Mar 5, 2023

BTW, the lint errors are fixed in the main branch. If you pull from the master it should be fixed.

@ajrazander ajrazander force-pushed the master branch 2 times, most recently from 5bc419a to 3ddd4a6 Compare March 8, 2023 03:27
@ajrazander
Copy link
Contributor Author

I removed the rxx and ryy gates and added the phase term you suggested to the already existing ms gate! :)

Added phase term to allow for MS gate
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.

Thank you for the updates! It looks good. Added a few suggestions.

You could use pytest qutip_qip/tests to run the test locally and check if they succeed. And black qutip_qip/src for the lint

src/qutip_qip/operations/gateclass.py Show resolved Hide resolved
src/qutip_qip/operations/gateclass.py Outdated Show resolved Hide resolved
src/qutip_qip/operations/gates.py Show resolved Hide resolved
tests/test_gates.py Show resolved Hide resolved
@ajrazander
Copy link
Contributor Author

ajrazander commented Mar 14, 2023

Thanks Boxi! I really appreciate your guidance through this!

Running black (23.1.0) on qutip-qip/src affected some files I'm not working on:
circuit/circuit.py, circuit/circuitsimulator.py, device/processor.py, qasm.py, qir.py, qiskit/backend.py, transpiler/chain.py

Should I not include these file changes in the commit?

@BoxiLi
Copy link
Member

BoxiLi commented Mar 14, 2023

Happy to help!

No, are you using the newest version of balck? It probably has something to do with the recent updates of black. Or you can also just fix those shown in the failed check below. Click on them and you will see the details.

@ajrazander ajrazander requested a review from BoxiLi March 22, 2023 01:56
@BoxiLi BoxiLi merged commit 363eb9d into qutip:master Mar 22, 2023
@BoxiLi
Copy link
Member

BoxiLi commented Mar 22, 2023

Merged! Thank you! @ajrazander Congrats!

I just notice that you were working on your master branch. It is almost always better to start a different development branch and keep your master synced with the upstream one. This could be helpful e.g. when you have multiple ongoing independent works. Also merging a PR may sometimes cause some conflicts between upstream/master and your own master.

@BoxiLi BoxiLi added this to the qutip-qip-0.3.0 milestone Jun 16, 2023
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.

2 participants