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

Implement cutting of general 2-qubit unitaries #302

Merged
merged 31 commits into from
Jul 26, 2023
Merged

Conversation

garrison
Copy link
Member

@garrison garrison commented Jul 2, 2023

Builds on #294. Closes #186. I'm marking as "on hold" at least until we merge the other explicit gate PRs.

Remaining action items

  • Release note
  • Rename supported_gates() -> explicitly_supported_gates()
  • update docstring
  • More comments/explanation
  • Update references in readme and docs

@garrison garrison added enhancement New feature or request cutting QPD-based circuit cutting code labels Jul 2, 2023
@garrison garrison added this to the 0.4.0 milestone Jul 2, 2023
@garrison garrison added the on hold Let's wait for something before merging label Jul 2, 2023
caleb-johnson
caleb-johnson previously approved these changes Jul 3, 2023
Copy link
Collaborator

@caleb-johnson caleb-johnson left a comment

Choose a reason for hiding this comment

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

So happy you figured this all out. I'm approving bc tests look good, but I'll be interested to hear how you connected the dots between the literature and the WeylDecomposer class in Qiskit. Thanks!

@caleb-johnson
Copy link
Collaborator

caleb-johnson commented Jul 3, 2023

Maybe not this PR, but should we provide a lookup table for Qiskit gates --> Sampling overhead, now that we suddenly support a lot of gates. Also, we now have a grab bag of optimal and non-optimal decomps in our supported gate set. How do we make this clear? Maybe two sections in the lookup table?

@caleb-johnson caleb-johnson dismissed their stale review July 3, 2023 17:01

PR still in draft status

@garrison
Copy link
Member Author

garrison commented Jul 5, 2023

Maybe not this PR, but should we provide a lookup table for Qiskit gates --> Sampling overhead

I agree -- it is worth having a section about this in the documentation. I could imagine this going in either the "explanation" section or the API references. It feels like a "reference"-type documentation to me, even though it is not strictly about the API.

Also, we now have a grab bag of optimal and non-optimal decomps in our supported gate set. How do we make this clear? Maybe two sections in the lookup table?

The authors of https://arxiv.org/abs/2006.11174v2 left open the possibility that their decomposition might not actually be optimal. (They discuss this in Sec. 2.4.) However, corollary 4.4 of https://arxiv.org/abs/2205.00016v2 actually shows that each of the gates that we explicitly support has an optimal decomposition. So, we are really left with two classes: decompositions that we can prove are optimal, and decompositions that may be optimal. Pretty much everything we explicitly mention in our table is going to fit in the first category; I can't think of a single "standard" gate that actually fits in the second. EDIT: actually, XXPlusYYGate and XXMinusYYGate might fit in the second category. I've so far been unable to work out a closed form expression for their overhead. (I haven't tried very hard yet, but I did try briefly.)

Base automatically changed from swap-gate to main July 5, 2023 20:22
@garrison
Copy link
Member Author

garrison commented Jul 5, 2023

We might as well put this on the 0.3 milestone, right? Deciding which release will influence whether I modify existing release notes or add new ones.

@caleb-johnson
Copy link
Collaborator

We might as well put this on the 0.3 milestone, right? Deciding which release will influence whether I modify existing release notes or add new ones.

Yes I thnk so

@garrison garrison modified the milestones: 0.4.0, 0.3.0 Jul 5, 2023
@garrison garrison removed the on hold Let's wait for something before merging label Jul 6, 2023
@coveralls
Copy link

coveralls commented Jul 19, 2023

Pull Request Test Coverage Report for Build 5663627358

  • 27 of 27 (100.0%) changed or added relevant lines in 1 file are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.08%) to 90.794%

Files with Coverage Reduction New Missed Lines %
circuit_knitting/utils/orbital_reduction.py 2 60.0%
Totals Coverage Status
Change from base Build 5590764933: 0.08%
Covered Lines: 2584
Relevant Lines: 2846

💛 - Coveralls

@@ -57,7 +57,7 @@
qpd.generate_qpd_weights
qpd.generate_qpd_samples
qpd.decompose_qpd_instructions
qpd.supported_gates
qpd.explicitly_supported_gates
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wanted to suggest optimally_supported_gates, but I can't convince myself I like it more. I think they are all optimal, at least?

Copy link
Member Author

@garrison garrison Jul 21, 2023

Choose a reason for hiding this comment

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

Yeah, they are all optimal -- even the ones for which we do not have explicit support.

Another option could be to get rid of this function -- or at least to make it private. The reason it was introduced in #277 was to support #278, but now that all two-qubit gates can be cut, it's no longer necessary. It might be nice to have something like this if we one day support some (but not all) 3-qubit gates (#258), which could be a case for keeping it but prefixing it with an underscore. This way, we'd at least have a use case in mind (again) before committing to support it.

By the way, all essential tests (i.e., all but a few specific ones) work if we take this PR and remove all the explicit gate support, i.e., if we apply the further patch:

diff --git a/circuit_knitting/cutting/qpd/qpd.py b/circuit_knitting/cutting/qpd/qpd.py
index 1e5be49..c6974a8 100644
--- a/circuit_knitting/cutting/qpd/qpd.py
+++ b/circuit_knitting/cutting/qpd/qpd.py
@@ -573,13 +573,6 @@ def qpdbasis_from_gate(gate: Gate) -> QPDBasis:
         ValueError: Cannot decompose gate with unbound parameters.
         ValueError: ``to_matrix`` conversion of two-qubit gate failed.
     """
-    try:
-        f = _qpdbasis_from_gate_funcs[gate.name]
-    except KeyError:
-        pass
-    else:
-        return f(gate)
-
     if isinstance(gate, Gate) and gate.num_qubits == 2:
         try:
             mat = gate.to_matrix()

so explicit support doesn't mean a ton anyway.

With #174, there will be one additional explicitly supported instruction: the Move instruction, which is not a Gate because it is not unitary. So it might be nice to have a function like the current one, somewhere. I'm leaning toward making the whole function private for now and removing it from the release notes until we better understand how we expect it to be used.

Copy link
Member Author

Choose a reason for hiding this comment

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

The more I thought about it, the more I felt like the most reasonable thing to do is to remove this function from the public API for now, hence my change in 8e95c4a.

It's not clear that this function remains useful now that we
support essentially all 2-qubit gates.  If we find a use for it
in the future, we can re-introduce it (or something like it) as a
public interface.
@garrison garrison linked an issue Jul 24, 2023 that may be closed by this pull request
11 tasks
@@ -18,7 +18,6 @@
decompose_qpd_instructions,
WeightType,
qpdbasis_from_gate,
supported_gates,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a 2-qubit gate in Qiskit that would break our cutting workflow if we tried to cut it?

I'm wondering if we can replace
if g in supported_gates():

with
if g.num_qubits == 2:

Copy link
Collaborator

Choose a reason for hiding this comment

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

I now see your docstring update below. They have to implement to_matrix

caleb-johnson
caleb-johnson previously approved these changes Jul 25, 2023
Copy link
Collaborator

@caleb-johnson caleb-johnson left a comment

Choose a reason for hiding this comment

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

Nice work and great documentation, thanks! LGTM 👍

@@ -18,7 +18,6 @@
decompose_qpd_instructions,
WeightType,
qpdbasis_from_gate,
supported_gates,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I now see your docstring update below. They have to implement to_matrix

theta: np.typing.NDArray[np.float64] | Sequence[float], /
) -> np.typing.NDArray[np.complex128]:
r"""
Exponentiate the non-local portion of a KAK decomposition.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Great writeup here, thanks

@garrison garrison merged commit d1db839 into main Jul 26, 2023
9 checks passed
@garrison garrison deleted the general-unitaries branch July 26, 2023 01:43
caleb-johnson pushed a commit that referenced this pull request Jul 28, 2023
* Add support for `SwapGate`

* Reorder terms

[ci skip]

* Add missing terms

* DRY the coefficients

* Fix coverage

* Add support for `iSwapGate`

* Fix black

* Add to release note

* Fix type hint

* Gates without parameters are nicer to work with

and can be singletons, one day!

* Remove a line

* Add comments describing channels

* `_copy_unique_sublists`

* Add `DCXGate`

* Tweak

* Implement cutting of general 2-qubit unitaries

Builds on #294.  Closes #186.

* Add tests of additional gates

* Fix type annotation

* Add explanatory comments

* `supported_gates()` -> `explicitly_supported_gates()`

* Add to references

* Improved error message and test about `to_matrix` conversion failing

* Add xref to `QPDBasis` in docstrings

* Add `qpdbasis_from_gate` to Sphinx build

* Make `explicitly_supported_gates` private and remove its release note

It's not clear that this function remains useful now that we
support essentially all 2-qubit gates.  If we find a use for it
in the future, we can re-introduce it (or something like it) as a
public interface.

* Fix intersphinx link

* Release note

* Update qpd.py: remove extraneous `from None`
caleb-johnson added a commit that referenced this pull request Aug 14, 2023
* Don't batch unnecessarily

* mypy

* black

* Missing varname

* fix bug

* Clean up code

* Improve comments

* release note

* release note

* Update batch-by-sampler-c4ae836df9997b1d.yaml

* Update batch-by-sampler-c4ae836df9997b1d.yaml

* Update batch-by-sampler-c4ae836df9997b1d.yaml

* Bump Python version in Dockerfile to 3.11 (#331)

Now that CKT supports Python 3.11, we might as well use the latest version in the Dockerfile.

* Fix formatting of example in `reduce_bitstrings` docstring (#332)

* Implement cutting of general 2-qubit unitaries (#302)

* Add support for `SwapGate`

* Reorder terms

[ci skip]

* Add missing terms

* DRY the coefficients

* Fix coverage

* Add support for `iSwapGate`

* Fix black

* Add to release note

* Fix type hint

* Gates without parameters are nicer to work with

and can be singletons, one day!

* Remove a line

* Add comments describing channels

* `_copy_unique_sublists`

* Add `DCXGate`

* Tweak

* Implement cutting of general 2-qubit unitaries

Builds on #294.  Closes #186.

* Add tests of additional gates

* Fix type annotation

* Add explanatory comments

* `supported_gates()` -> `explicitly_supported_gates()`

* Add to references

* Improved error message and test about `to_matrix` conversion failing

* Add xref to `QPDBasis` in docstrings

* Add `qpdbasis_from_gate` to Sphinx build

* Make `explicitly_supported_gates` private and remove its release note

It's not clear that this function remains useful now that we
support essentially all 2-qubit gates.  If we find a use for it
in the future, we can re-introduce it (or something like it) as a
public interface.

* Fix intersphinx link

* Release note

* Update qpd.py: remove extraneous `from None`

* Improve the instructions regarding pandoc (#336)

I've also tried to make the developer documentation easier to discover

* Make the repository link more obvious from the Sphinx build (#338)

* Make the repository link more obvious from the Sphinx build

* Enable "edit" link in the header

* Add comment

* Add README badge linking to stable documentation (#339)

* Add docs badge to link to stable docs

* Add ruff badge

* Remove ruff

* Update README.md (#340)

Making directions on opening the docs a little more succinct.

* Change var name

* Add SECURITY.md (#337)

* Implement wire cutting as a two-qubit instruction (#174)

* Implement wire cutting as a two-qubit instruction

* Update type annotation

* s/gate/instruction/

* Add overhead test for `Move` instruction

* Add wire cutting tutorial

* Add `Move` to Sphinx build

* Doc updates suggested by Caleb

* Add release note and link to new tutorial

* Clarify wording

following #174 (comment)

* Improvements to `Move` docstring

* Use svg as the plot format

This avoids pixelation on high-dpi displays

* Remove unnecessary uses of `CircuitInstruction`

https://github.com/Qiskit-Extensions/circuit-knitting-toolbox/pull/174/files#r1278067109

* The notebook tests should ignore any files that crop up in `docs/_build`

`matplotlib.sphinxext.plot_directive` likes to leave python files there

---------

Co-authored-by: Jim Garrison <garrison@ibm.com>
Co-authored-by: Ibrahim Shehzad <75153717+IbrahimShehzad@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cutting QPD-based circuit cutting code enhancement New feature or request
Projects
None yet
3 participants