-
Notifications
You must be signed in to change notification settings - Fork 82
Pennylane-Qualtran interoperability #1559
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
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
mpharrigan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice! will probably have to add some tests. We'll also need to add pennylane to the requirements, which I can help with (since we have a convoluted system for pinning the CI dependencies)
For the tests, I'm not sure where I'm supposed to add them. I took a look |
|
Yeah, that's a good question. Usually we'd put them alongside the code for the adapter classes; but that will reside in the Pennylane repository. I think in this case it will suffice to have some basic unit tests for the particular examples for which you implemented the method (which can live in the test files associated with those bloqs) |
|
You can also add a test to |
|
Hi @mpharrigan, I added some very basic unit tests for each of the atomic bloqs I wrote an |
|
Sorry, since you're a first-time contributor I have to press the "approve running the CI" button every time you push a commit, sadly |
|
You can run the various scripts in the check/format-incremental --applyit will apply the formatter to your code check/mypy
check/pylintwill report typecheck and lint errors (but you have to fix thme) |
|
oh, I have to update our dependencies. I will do that tomorrow |
|
Can the [WIP] tag be removed from the title of the PR? |
Perhaps the workflow file should be edited instead for this PR specifically to get the CI passing. We can then remove it when qml.FromBloq is merged into pennylane master. |
|
Yes, I can update our dependencies to that specific branch for the time being. I'll open an issue to switch it back to a proper release after the qualtran features are picked up in a release. Can you ensure that that branch will survive until the next pennylane release? |
|
alright, if you merge in |
Awesome thanks. Yes the branch will survive until the next pennylane release. I'll let you know when it's ready to merge and we can coordinate the process. |
Jaybsoni
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me for the most part. I left a few comments to be addressed. Otherwise its good from my side 👍🏼
One question for @mpharrigan, I am not sure how paranoid we should be about conventions? For most of the tests all we are checking is that if we map a Bloq using FromBloq, we get the expected PL operator. We DONT check if the Qualtran definition of an operation is the same as the PL one. For example, I noticed in the global_phase the extra factor of pi that is used in Qualtran. Should we compare the matrix produced via tensor_contract against the qml.matrix function?
7d1b329 to
9c136f1
Compare
My opinion is that we should test that they're equivalent (where feasible). In the global phase op, the |
Yes, the |
Yes the |
For the tests, I purposefully avoided comparing tensor_contract with matrix because the behaviour of the two functions are not 100% equivalent. The shape of PL's matrix always has the shape of (2^len(wires), 2^len(wires)) whereas that's not true for tensor_contract. E.g. for GlobalPhase, the matrix() will be whereas for qualtran's GlobalPhase().tensor_contract(), we can expect: Regardless (cant hurt to have more tests), I've added a set of assertions to compare that the PL matrix matches the QT tensor_contract for most of the bloqs. For global phase, I used a manual test case instead of comparing directly to QT's tensor_contract. |
|
I think this PR looks pretty much ready. If you are okay with the PR @mpharrigan please merge 😺 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good from me 👍🏼
In this PR, we set out to define a new method
as_pl_op()for the baseBloqclass as well as several "basic gates" that have direct 1:1 translations with PennyLane operators. This is a work in progress and relies on another PR inside of PennyLane that contains theqml.FromBloqadapter class. We open this now for visibility and discussion purposes, but the work is still on-going.