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

Add user defined gates, MS gate and fix some bugs #1025

Closed
wants to merge 16 commits into from

Conversation

BoxiLi
Copy link
Member

@BoxiLi BoxiLi commented Jun 21, 2019

  1. expand_oper: expand a qubits operator to dim N
  2. User-defined gate can be given by a python function. By passing a list of user-defined gate function to the attribute QubitCircuit.user_gate, QubitCircuit.propagators() can return the corresponding matrices. Resolve Custom gates for QubitCircuit #914
  3. MS gate and physical single qubit rotation. (Is the name mc_gate and qrot appropriate?) Resolve td qobj #814
  4. Fix some bugs in resolving circuit gates. Allow more than one 2-qubits gates in the basis. This is actually already required in spinchain.py but covered by a bug

quantshah and others added 10 commits June 11, 2019 22:32
* import + sesolve

* psi in args part 1

* psi in args part 2

* state in args part3

* dyn args test1

* dyn args test2

* args debug

* ready for tests

* s/mesolve pass tests

* mcsolve rework

* mcsolve rework part 2

* mcsolve cython cleaning

* mcsolve working

* mcsolve cython as object

* mcsolve pass all test

* automated test almost passing

* pass all tests

* docs and cleaning

* diag ok

* clean propagator

* clean propagator 2

* clean floquet

* clean floquet 2

* mcsolve serial_map bugfix

* object args bug correction

* dense cQobjEvo bugfix

* spliting PR

* merge priority correction

* merge priority correction

* mcsolve based on qoevo

* better rouchon citation

* final state avg?

* faster postprocessing

* final_state average flag

* final state runs as property

* Some patches for qip.Gate (qutip#995)

* remove repetitive assignment

* move description to class def

Doc won't show it if it's under __init__

* check invalid input for targets and control

* all must be integer

* NotImplemented is not callable, use NotImplementedError

* add whitespace

* refactor

* format correction
@coveralls
Copy link

coveralls commented Jun 21, 2019

Coverage Status

Coverage decreased (-0.06%) to 71.698% when pulling 10f02d0 on BoxiLi:user_gate into f9999c9 on qutip:master.

@nathanshammah nathanshammah self-requested a review June 23, 2019 13:54
@nathanshammah nathanshammah added the review in progress PR being reviewed label Jun 23, 2019
qutip/qip/gates.py Outdated Show resolved Hide resolved
@@ -177,6 +177,27 @@ def phasegate(theta, N=None, target=0):
dims=[[2], [2]])


def qrot(theta, phi, N=None, target=0):
Copy link
Member

Choose a reason for hiding this comment

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

what about being a bit more explicit and call it qubit_rotate, qrotate, or rotate_qubit? Maybe qrot` is fine too.

Parameters
----------
theta : The duration of the interaction pulse
"""
Copy link
Member

Choose a reason for hiding this comment

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

Also here: Shouldn't it have also "Returns" in the docstring?

Copy link
Member

Choose a reason for hiding this comment

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

Add target docstring. Maybe default should be a list instead of a tuple for coherence with rest of code where integer is integer or list?

Copy link
Member

@nathanshammah nathanshammah left a comment

Choose a reason for hiding this comment

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

Regarding the Molmer-Sorensen gate: It is very nice how you handled the possibility for expansion to N qubits. I could not test the code as I could not figure out how to fork properly the PR to the branch. If you add a short gist with a brief example of both the Molmer Sorensen gate and creating a user-defined gate, maybe I can copy paste this more easily and check it out. It think such an example would make an interesting addition to qutip.org/tutorials.

qutip/qip/gates.py Show resolved Hide resolved
qutip/qip/gates.py Show resolved Hide resolved
Parameters
----------
theta : The duration of the interaction pulse
"""
Copy link
Member

Choose a reason for hiding this comment

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

Add target docstring. Maybe default should be a list instead of a tuple for coherence with rest of code where integer is integer or list?

qutip/tests/test_gates.py Show resolved Hide resolved
qutip/qip/gates.py Outdated Show resolved Hide resolved
qutip/qip/gates.py Outdated Show resolved Hide resolved
qutip/qip/gates.py Outdated Show resolved Hide resolved
@nathanshammah
Copy link
Member

Also: maybe this PR, which seems self-contained with respect to deliverables and independent on the "qutip.nisq" module, could be made directly to the master, so that we can already include it in the next QuTiP release.

@BoxiLi
Copy link
Member Author

BoxiLi commented Jun 23, 2019

@nathanshammah Thanks a lot for the review. I'm not sure how gist works, should I upload only the example or with the definition of expand_oper? Maybe we can talk about this tomorrow at the meeting. I have some examples in notebook.
Yes, you are right, I think this PR could be merged directly to master, but I also used expand_oper in my circuitprocessor a lot so I may need you to update qip_opt from master branch.

@nathanshammah
Copy link
Member

Maybe we can change the base branch to master. Never did it before but it seems straightforward https://help.github.com/en/articles/changing-the-base-branch-of-a-pull-request and then update qip opt branch.

@BoxiLi BoxiLi changed the base branch from qip_opt to master June 24, 2019 14:57
@BoxiLi
Copy link
Member Author

BoxiLi commented Jun 24, 2019

@nathanshammah @quantshah I tried it, but a lot of commits appear which don't belong to me. They are updates from the master branch.

@quantshah
Copy link
Member

Great. I was trying to see if I can add commits to this PR directly and it is possible. I will post here how to do that in a while. Just going to resolve these conflicts and rebase against master

@BoxiLi
Copy link
Member Author

BoxiLi commented Jun 24, 2019

@quantshah I find it a bit weird because those commits are merged from master to qip_opt and I'm now just trying to merge back to master. Where do those conflicts come from?

@quantshah
Copy link
Member

The conflicts are because first we branched qip_opt from master, but then qip_opt was not updated to master itself and I tried to update it against master since this will be a PR which gets merged into master and not your GSoC project. This led to many changes which happened over master and were not there in your PR and led to some conflicts between master and qip_opt I suppose.

@quantshah
Copy link
Member

We could take this PR as an example of how to work with a very messy git history. I see some forced pushes which should probably have been rebases to avoid conflicts.

But at this point I would suggest to Boxi that you open a fresh PR branching from the current master on your local qutip repo. So do:

git checkout master
git pull upstream master
git push origin master

This syncs your master to QuTiP. Then, start a new branch and make the same changes that you did here on that new branch.

Make the changes, add commit etc.

git checkout -b user_gate2
...
git add .
git commit -m "message"

and then make a new PR and close this (without merging). Then delete this branch (locally and remote)

git branch -D user_gate
git push origin -delete user_gate

@BoxiLi
Copy link
Member Author

BoxiLi commented Jun 24, 2019

But if I haven't changed any of those files, shouldn't git ignore them and believe that master is the up-to-date one? May the workflow here "master->qip_opt->user_gate->master" is not what git expected. The correct way should be "master->qip_opt->user_gate->qip_opt->master" or "master->user_gate->master".
So I should have started from master if I want to be merged back to master, not through qip_opt.

@BoxiLi
Copy link
Member Author

BoxiLi commented Jun 24, 2019

Sure, I'll do it.

@quantshah
Copy link
Member

So I should have started from master if I want to be merged back to master, not through qip_opt.

It is not necessary to do so. You just have to make sure that the branch you want to merge to (in this case we changed this midway to master from opt_qip) is not having conflicts with your code. And maybe that is what happened. Not sure though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review in progress PR being reviewed
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Custom gates for QubitCircuit
4 participants