Adding Type2 Decompositions #240
Conversation
This change adds the Type2 decompositions and refactors the Qsharp.Core into Qsharp.Core + Qsharp.Base to allow other core packages to be built on top of Base.
swernli
requested review from
ScottCarda-MS,
anpaz,
bamarsha,
bettinaheim,
cesarzc,
cgranade and
efratshabtai
src/Simulation/DecompositionsCore.Test/Common/IntrinsicExecute.qs
Outdated
Show resolved
Hide resolved
Co-authored-by: Scott Carda <55811729+ScottCarda-MS@users.noreply.github.com>
| @@ -0,0 +1,137 @@ | |||
| namespace Microsoft.Quantum.Intrinsic { | |||
There was a problem hiding this comment.
I am confused about what functionality is considered to be 'utility' versus 'decomposition' versus 'a rule to transform the circuit'. Isn't DispatchR1Frac a decomposition? Will it be valid to use these rules with any future decomposition package?
There was a problem hiding this comment.
This is an excellent point. Some of those divisions were purely a side-effect of shuffling operations around to get the package structure to compile, while others were motivated by what would be shared with the next decomposition package. However, with only one decomposition package in the changes it's hard to see where sharing utility functions makes sense. This is part of the motivation for breaking this into different PRs/commits which show more of the relationships between pieces.
In addition, your suggestion during design discussion about breaking the decompositions into smaller reusable chunks is a great one, and has given me a lot to think about. Based on that suggestion, I went back to the drawing board a bit and think I will rearrange a lot of the structure here, hopefully in a way that makes it clearer which pieces can be reused well vs are specific to a particular decomposition/gate-set.
| @@ -0,0 +1,271 @@ | |||
| // Copyright (c) Microsoft Corporation. All rights reserved. | |||
| // Licensed under the MIT License. | |||
There was a problem hiding this comment.
Maybe, add a file level comment explaining what it means to nominate some of the gates as 'intrinsic' (as far as decompositions are concerned)?
There was a problem hiding this comment.
Yes, this whole approach to decompositions needs a lot more documentation, both in terms of comments in the code and readme files. I added a specific task on #249 regarding documentation to make sure that gets incorporated in the feature branch before merging to master.
| @@ -0,0 +1,67 @@ | |||
| // Copyright (c) Microsoft Corporation. All rights reserved. | |||
| // Licensed under the MIT License. | |||
There was a problem hiding this comment.
How do these functions differ from those in CircuitUtilities? For example, why 'TS' helper would be specific to Type2 decomps?
There was a problem hiding this comment.
In some of the experiments with decomposition packages, the other package had a different TS helper implementation, but that's impossible to see when only one package is in the changes. I'll work on the form and function of these helpers as I adjust my decomposition approach.
| /// This is a no-op. It is provided for completeness and because | ||
| /// sometimes it is useful to call the identity in an algorithm or to pass it as a parameter. | ||
| @EnableTestingViaName("Test.Decompositions.I") | ||
| operation I(target : Qubit) : Unit |
There was a problem hiding this comment.
operation I [](start = 4, length = 11)
Why is Identity not intrinsic? Oh, the 'intrinsic' in the decomposition context means 'natively supported by the target of this type', right? So, those gates are listed in Type2-Intrinsic.qs and here we are providing decompositions from Q# intrinsic gates into those supported by the target...
There was a problem hiding this comment.
Yup, you nailed it. Again, documentation will help here, but there it's easy to have confusion between "intrinsic" meaning an operation or function that has body intrinsic in its implementation, "intrinsic" meaning a member of the Microsoft.Quantum.Intrinsic namespace, "intrinsic" meaning natively supported by the target simulator/hardware, and "intrinsic" meaning part of the default set of body-intrinsic operations in the Q# core. I'll clarify this in the docs, but for reference here, this is how I'm thinking about terminology:
- Quantum Gate Set: the set of gates natively supported by the underlying simulator/hardware.
- Body-Intrinsic: any operation or function whose Q# implementation is
body-intrinsic. When an operation in the Microsoft.Quantum.Intrinsic namespace is also body-intrinsic it is often an indication that it represents a gate from the quantum gate set, but not always, as some helper methods that rely on C# implementation are body-intrinsic, but that should be rare in a decomposition package. - Decomposition: any operation representing a quantum gate or operation that is not natively supported by the target simulator/hardware and so must be implemented by a combination of calls to helpers and body-intrinsic operations. From a circuit perspective, the decomposition is often the circuit comprised of supported gates needed to implement an unsupported gate.
- Default Intrinsics: the set of body-intrinsic operations defined in the Qsharp.Core that are part of the Microsoft.Quantum.Intrinsic namespace. Right now this is equivalent to the QuantumSimulator's quantum gate set, but that may change as simulator implementation grows and changes.
There was a problem hiding this comment.
Thank you for the explanation! In general, I'd very much prefer using different terms for the different kind of intrinsics than relying on the documentation to describe the different meaning depending on the context. I realize that some of the terminology has been around for a bit and it would be hard to change it, but targeting is relatively recent -- maybe we could introduce a new term here?
There was a problem hiding this comment.
Hmm... I do agree that would be nicer, but two major limiting factor are that I need to keep the namespace the same and I can't reasonably change the body intrinsic keywords. So the language sort of has one of those confusions built-in. I'm not going to give up though... I firmly believe that having good terminology is crucial to being able to discuss engineering problems and solutions; if we agree how to communicate the ideas then we can't really collaborate on them well. Definitely let me know if you have any ideas for alternate, more specific terminology!
| @@ -0,0 +1,71 @@ | |||
| namespace Microsoft.Quantum.Decompositions.Utilities { | |||
There was a problem hiding this comment.
This is yet another utilities file :) What is the guiding principle re which utilities should go here and which into CircuitUtilities.qs? How will our future selves discover that a utility for a particular task already exists?
There was a problem hiding this comment.
For most of this, the guiding principal has just been "what reduces duplication and still compiles" but I will go back to the drawing board and think about what principals and patterns this should establish.
This is the kind of usage of 'Core' my intuition is keyed off: something nobody should mess with because it's meant to be agnostic of whatever variations your project/application supports... Refers to: src/Simulation/QsharpBase/Core.cs:7 in 38458d0. [](commit_id = 38458d0, deletion_comment = False) |
| // Copyright (c) Microsoft Corporation. All rights reserved. | ||
| // Licensed under the MIT License. | ||
|
|
||
| namespace Microsoft.Quantum.Intrinsic { |
There was a problem hiding this comment.
namespace Microsoft.Quantum.Intrinsic [](start = 0, length = 38)
Utils.qs has a comment about the special status of this namespace. Do we have other places when we add functionality into this namespace? Is it really useful to split it across multiple files? At the very least, I'd suggest duplicating there the namespace comment.
There was a problem hiding this comment.
The comment at the top of the namespace in Utils.qs is an API documentation comment, such that repeating it in both places would lead to incorrect output documentation. Standard practice in the libraries has been to have a Properties/NamespaceInfo.qs with the namespace API documentation comment for this reason.
There was a problem hiding this comment.
I'll make sure to thread the needle between namespace/operation/function documentation intended for callers that should be part of the API docs vs documentation for developers editing the implementation of the API. Some of that will be moved out to readme files in the appropriate folders, and for other comments that need to stay close to the code I can move it inside the operation/function scope to differentiate from doc comments.
|
|
||
| namespace Test.Decompositions | ||
| { | ||
| public class Type2Simulator : QuantumSimulator |
There was a problem hiding this comment.
QuantumSimulator [](start = 34, length = 16)
Out of curiosity: when would one use QuantumSimulator versus IQuantumProcessor for test purposes?
There was a problem hiding this comment.
For now, these are extensions on the existing simulator implementation, which does not actually implement IQuantumProcessor. That is intentional because I need to still have the existing simulator implementations of the default intrinsics from Qsharp.Core to be present alongside the test-renamed intrinsics from the decomposition package. More comments will definitely help explain this...
|
Stefan, do you think you could add a readme at the root of DecompositionsCore folder explaining the purpose of decompositions and the logic for resolving user's Q# code when specific decomposition packages are used? The readme could also introduce terminology that then could be used in file-level comments. |
| /// This namespace includes Q# intrinsic utilies. | ||
| /// | ||
| /// # Remarks | ||
| /// This namespace is opened automatically by the Q# compiler, so all |
There was a problem hiding this comment.
Is this correct? If so that would be a significant and breaking change. The Microsoft.Quantum.Intrinsic namespace is not currently opened automatically; only Microsoft.Quantum.Core.
There was a problem hiding this comment.
I agree, I think this doc comment is incorrect. I can clean that up.
| // Copyright (c) Microsoft Corporation. All rights reserved. | ||
| // Licensed under the MIT License. | ||
|
|
||
| namespace Microsoft.Quantum.Intrinsic { |
There was a problem hiding this comment.
The comment at the top of the namespace in Utils.qs is an API documentation comment, such that repeating it in both places would lead to incorrect output documentation. Standard practice in the libraries has been to have a Properties/NamespaceInfo.qs with the namespace API documentation comment for this reason.
| is Adj + Ctl { | ||
| body (...) { } | ||
| adjoint self; | ||
| } |
There was a problem hiding this comment.
| is Adj + Ctl { | |
| body (...) { } | |
| adjoint self; | |
| } | |
| is Adj + Ctl { } |
There was a problem hiding this comment.
Good suggestion! I think this makes the no-op nature of I more transparent than the existing formulation.
|
Thanks everyone for the great comments and feedback! I filed #249 to capture/track this work, and based on some of the feedback and design discussions, I will be breaking this change apart into a few PRs to a feature branch, each with more targeted focus. I'll still respond to the comments here so we can capture the discussion and to help guide my approach to the next PR. |
This set of changes adds the Type2 decompositions to the runtime. It also includes the splitting of Qsharp.Core into Qsharp.Core + Qsharp.Base, which allows other packages to define a replacement core with alternate intrinsics on top of Base. The Type2 decompositions are the first example of such an alternate core.