Adding Type2 targeting package and tests #363

swernli · 2020-08-28T21:27:23Z

This changes continues building on the work in feature/decomp to enable targeting alternate gate sets via packages specifying the supported set of intrinsics and decompositions for that gate set. This introduces Type2, which targets a set of intrinsics that includes SWAP and the 3 Pauli Ising gates. It also adds support for executing against this target package in the QDK simulator, and shares some of the existing intrinsic unitary tests to validate behavior of the package.

See #249 for the rundown on what work is part of this overall feature.

ScottCarda-MS

I've just skimmed through it, but it looks good to me. Seems that the vast majority of the changes here are just due to moving things around in the file structure and under different namespaces.

swernli · 2020-08-28T22:04:59Z

One thing these changes do not yet support is the EntryPointDriver, which for now pulls in the QsharpCore-based simulator as a compilation dependency. I think there is a way to refactor it such that it takes a generic dependency on SimulatorBase and then uses which ever simulator comes in the target package the executable was compiled against, but I'm not sure how to make that work with the Toffoli simulator and Resource Estimator as default options (those are not present in Type2 yet). This shouldn't affect the current PR, but definitely something we need to figure out as part of surfacing targeting packages as a finished feature.
@SamarSha for FYI.

swernli · 2020-08-28T22:38:27Z

To make this large and unwieldy PR a little bit easier to review, I'll call out the files that a new or significantly modified here...

ScottCarda-MS

reset review

bamarsha · 2020-08-28T23:29:42Z

One thing these changes do not yet support is the EntryPointDriver, which for now pulls in the QsharpCore-based simulator as a compilation dependency. I think there is a way to refactor it such that it takes a generic dependency on SimulatorBase and then uses which ever simulator comes in the target package the executable was compiled against, but I'm not sure how to make that work with the Toffoli simulator and Resource Estimator as default options (those are not present in Type2 yet). This shouldn't affect the current PR, but definitely something we need to figure out as part of surfacing targeting packages as a finished feature.

Does this mean EntryPointDriver will need to create the simulator via reflection in the same way that it creates the target quantum machine for Azure Quantum?

swernli · 2020-08-28T23:49:05Z

Does this mean EntryPointDriver will need to create the simulator via reflection in the same way that it creates the target quantum machine for Azure Quantum?

That’s certainly one possibility, yeah. The other idea I had but haven’t tried out yet is reworking the code so that the entry point driver only depends on SimulatorBase and then it just has a reference to the common package. Then each simulator package would pull in the entry point driver as a dependency, instead of the other way around.

bamarsha · 2020-08-28T23:58:57Z

Then each simulator package would pull in the entry point driver as a dependency, instead of the other way around.

That seems strange, since it's possible to use the simulators without needing the command-line interface that the EntryPointDriver provides? However, another option is that the Q# executable project would depend on both a simulator package and the driver package, and give simulator factories to the driver at runtime.

How many different simulators will there be? My understanding was that we would still have just one simulator for all execution targets, is that incorrect?

swernli · 2020-08-30T05:25:38Z

That seems strange, since it's possible to use the simulators without needing the command-line interface that the EntryPointDriver provides? However, another option is that the Q# executable project would depend on both a simulator package and the driver package, and give simulator factories to the driver at runtime.

That's a cool idea. I was trying to think about how something like that might work, so I would really appreciate any ideas or advice you have on the design there.

How many different simulators will there be? My understanding was that we would still have just one simulator for all execution targets, is that incorrect?

Unfortunately, I'm disappointed by how this part of the work turned out. I was hoping to have a single simulator that would support all targets, and I still think we should keep an eye out for the opportunities to refactor everything into one simulator to handle all targeting packages. I'll give a summary here of the issues I ran into with supporting multiple targeting packages on the same simulator...

Each targeting package collects the subset of intrinsics and decompositions that comprise it. The simulator needs to implement the intrinsics in C# so that they can wrap the appropriate calls into the native C++ simulator. However, some operations that are intrinsic for one package are decompositions for another, and in that case if the compiled simulator included C# implementations of those operations they would end up overriding the corresponding decomposition. To pick a specific example, Exp is an intrinsic operation for the QsharpCore package and so has an implementation in the simulator that inherits from the Q# declaration, public class QSimExp : Quantum.Intrinsic.Exp. This works because the body intrinsic Exp is just an abstract base class that gets overridden by the simulator implementation. However, Exp in the Type2 package is supposed to be a decomposition on top of the Ising gates, so it is fully implemented in Q# instead of abstract. But if that package is built along with the core simulator, the implementation of QSimExp that defines itself as a child of the Exp class ends up overriding the decomposition with the wrapper for the native simulator. Technically, the circuit will be run correctly, but the decomposition does not get used, so anything like qubit counts or gate counts won't really be correct, and tracing the execution will be confusing. If the goal of a simulator is just to calculate the overall result of the quantum algorithm by any means then this doesn't really matter. But if the goal is to actually simulate the sequence of operations defined by the Q# then it should include the defined decompositions and not take any short cuts. I tried to find a way around this, but as long as the design for intrinsic implementation uses inheritance and overrides I don't think we can avoid this problem. The only path forward I could come up with is ensuring that the simulator for a given target package only includes implementations for that package's intrinsics and nothing else, which necessitates a simulator per package. At least by utilizing the same patterns I used for the targeting packages we can avoid duplicating the implementation, but there will still have to be separate projects for each simulator to match the separate target packages.

swernli · 2020-08-31T18:23:08Z

@SamarSha Rather than figure the entry point handling all as part of this PR, I've added it as an additional ToDo on the #249 so that I can ensure it gets addressed before this feature branch is merged into master. I can try a couple of the ideas we've had so far and get that together as a separate PR soon.

bamarsha · 2020-08-31T18:25:20Z

That's a cool idea. I was trying to think about how something like that might work, so I would really appreciate any ideas or advice you have on the design there.

Sure, I haven't thought that much about it but I could elaborate if that'd be helpful, maybe it would be easier to chat more about it offline.

Unfortunately, I'm disappointed by how this part of the work turned out. I was hoping to have a single simulator that would support all targets, and I still think we should keep an eye out for the opportunities to refactor everything into one simulator to handle all targeting packages. I'll give a summary here of the issues I ran into with supporting multiple targeting packages on the same simulator...

Thanks for the explanation. If I understood correctly, does that mean that if the simulator could tell if a particular operation had a Q# implementation or was an intrinsic, and could decide whether to implement something based on that information, then there could be only one simulator for all targets? (Or equivalently, if a Q# implementation of an operation had higher priority over the simulator implementation?)

swernli · 2020-08-31T18:47:51Z

If I understood correctly, does that mean that if the simulator could tell if a particular operation had a Q# implementation or was an intrinsic, and could decide whether to implement something based on that information, then there could be only one simulator for all targets? (Or equivalently, if a Q# implementation of an operation had higher priority over the simulator implementation?)

Yes, that is exactly what we would need. Inheritance with abstract methods and overrides doesn't give us a way to do that. The only mechanism C# offers that prevents a given implementation of a method from being overridden is sealed but rather than suppressing the child class override it would result in a compile time failure, which also doesn't work for this usage. I did spend some time trying to think of a schema whereby the generated classes are actually children of the simulator classes instead of the other way around such that the generated implementations always take precedence, but I didn't come up with a reasonable way to make such a hierarchy work.

alan-geller · 2020-08-31T19:35:49Z

If I understood correctly, does that mean that if the simulator could tell if a particular operation had a Q# implementation or was an intrinsic, and could decide whether to implement something based on that information, then there could be only one simulator for all targets? (Or equivalently, if a Q# implementation of an operation had higher priority over the simulator implementation?)

Yes, that is exactly what we would need. Inheritance with abstract methods and overrides doesn't give us a way to do that. The only mechanism C# offers that prevents a given implementation of a method from being overridden is sealed but rather than suppressing the child class override it would result in a compile time failure, which also doesn't work for this usage. I did spend some time trying to think of a schema whereby the generated classes are actually children of the simulator classes instead of the other way around such that the generated implementations always take precedence, but I didn't come up with a reasonable way to make such a hierarchy work.

I think you can override the current mechanism by modifying the InitBuiltinOperations method on SimulatorBase, or alternatively by changing the simulator class initialization and registering the Q# decompositions as overrides (using AbstractFactory.Register).

swernli · 2020-08-31T19:41:13Z

I think you can override the current mechanism by modifying the InitBuiltinOperations method on SimulatorBase, or alternatively by changing the simulator class initialization and registering the Q# decompositions as overrides (using AbstractFactory.Register).

Oh, that's an interesting idea! I think I could try enlightening InitBuiltinOperations to only register overrides of base types that are abstract, and then the simulator could be a single implementation. Thanks @alan-geller, I'm going to try that now!

swernli · 2020-09-01T20:36:54Z

@alan-geller Ok, I tried the approach you suggested and still didn't quite work, but it shows promise. The issue I ran into is that the simulator still defines it's classes as inheriting from the intrinsic operations, as in class QSimExp : Quantum.Intrinsic.Exp. This means that the simulator project needs to be compiled with references to a targeting package that defines those classes. But once it's compiled with one (either QsharpCore or Type2Core) it cannot later be pulled in as a dependency with the other. When I try to do that, in the tests for example, it will complain that it can't resolve the conflict between the identically named classes in the two dlls. I was thinking I could try to disconnect the simulator from the core package by having the intrinsic implementations inherit directly from Unitary and ICallable, but then that would require reworking the way the intrinsics are discovered and registered in the simulator initialization and it would require the simulator classes define all the class methods instead of just the abstract ones, all while loosening the type safety we have right now. I'm likely going to adapt this change and push it up anyway, but for now it will still require two simulator projects instead of just one, which doesn't really fix the problem we were hoping to address.

This reverts commit 2559b13.

swernli · 2020-09-02T18:29:59Z

Turns out dotnet does not support multiple .csproj files in the same folder, so my attempts at a clever combination of the two simulator folders was only accidentally working. Thankfully it failed in the Build mac pipeline to draw my attention to that; the mac dotnet must more aggressively parallelize the builds, preventing that approach from "working" as consistently.

swernli · 2020-09-02T18:57:39Z

@alan-geller @SamarSha I think I've gotten these changes to the local maximum for the time being. Please let me know if you've got any other feedback or ideas for me to follow up on. I'm hoping I can get this merged today and move to the next target package. By way of summary, here's the issues we've identified as out of scope for this PR that I hope to handle later:

EntryPointDriver should not have a compile-time dependency on the simulator, as that would require a new driver per target package/simulator package. I added this to the task list in Q# should include decompositions to alternate quantum gate sets #249 so it gets addressed as part of finishing this feature.
One simulator should be able to act as implementation for many target packages. This is a challenging design problem, as the current use of inheritance means we must compile a simulator per target package. We have some ideas about how this might be address, but so far all of them imply some redesign and heavy reliance on some new stub generation (hopefully helped by Alan's new tooling).

alan-geller

A few comments, nothing huge. I really like the overall PR, I think it really cleans up a lot of our infrastructure a lot.

alan-geller · 2020-09-03T16:43:14Z

src/Simulation/Simulators.Type2/FindNuspecReferences.ps1

+# This is problematic because we currently don't want to create a package for every dll.
+#
+# On the other hand, when creating a package using nuget pack, nuget does not
+# identifies PackageReferences defined in the csproj, so all the dependencies


Suggested change

# identifies PackageReferences defined in the csproj, so all the dependencies

# identify PackageReferences defined in the csproj, so all the dependencies

alan-geller · 2020-09-03T16:45:17Z

src/Simulation/Simulators/QCTraceSimulator/Circuits/Primitive.IsingXX.qs

+// Licensed under the MIT License.
+
+namespace Microsoft.Quantum.Simulation.Simulators.QCTraceSimulators.Circuits {
+    operation IsingXX (theta : Double, qubit0 : Qubit, qubit1 : Qubit) : Unit is Ctl {


Why not Adj + Ctl? Exp is Adj+Ctl, so this could be, unless there's a reason for it not to be.

I've fixed these to all be Adj as well. Thanks!

alan-geller · 2020-09-03T16:46:01Z

src/Simulation/Simulators/QCTraceSimulator/Circuits/Primitive.IsingYY.qs

+// Licensed under the MIT License.
+
+namespace Microsoft.Quantum.Simulation.Simulators.QCTraceSimulators.Circuits {
+    operation IsingYY (theta : Double, qubit0 : Qubit, qubit1 : Qubit) : Unit is Ctl {


Why not Adj + Ctl? Exp is Adj+Ctl, so this could be, unless there's a reason for it not to be.

alan-geller · 2020-09-03T16:46:46Z

src/Simulation/Simulators/QCTraceSimulator/Circuits/Primitive.IsingZZ.qs

+// Licensed under the MIT License.
+
+namespace Microsoft.Quantum.Simulation.Simulators.QCTraceSimulators.Circuits {
+    operation IsingZZ (theta : Double, qubit0 : Qubit, qubit1 : Qubit) : Unit is Ctl {


Why not Adj + Ctl? Exp is Adj+Ctl, so this could be, unless there's a reason for it not to be.

alan-geller · 2020-09-03T16:49:38Z

src/Simulation/Simulators/QuantumSimulator/Extensions.cs

                select op;

-            foreach (var op in ops)
+            foreach (var op in ops.Where(o => o.BaseType.IsAbstract))


This won't work for C# overrides of Q# operations; that is, for emulation. Emulations are almost always done with an explicit call to Register today, so I think it's OK for now, but I'm not sure this is the right answer for the long term; we need to think more about it.
I think this is covered by one of the associated issues you've opened.

alan-geller · 2020-09-03T20:01:42Z

src/Simulation/TargetDefinitions/Decompositions/R.qs

+    /// when used with the `Controlled` functor.
+    @EnableTestingViaName("Test.TargetDefinitions.R")
+    operation R (pauli : Pauli, theta : Double, qubit : Qubit) : Unit is Adj + Ctl {
+        if( pauli != PauliI ) {


Just a style question: I would do this as a top-level if with cases for all 4 Paulis, rather than the way you've structured it. Either are correct, of course; I suspect that there's a (probably negligible) perf benefit to my approach, with I as the final else case, because I suspect I is the least common case, but I might be wrong. I'm curious why you structured it this way.

Good point! This code is adapted from some earlier research code, and I didn't notice that refactoring opportunity. I agree that a single if-elif-else statement would work better!

alan-geller · 2020-09-03T20:44:06Z

src/Simulation/TargetDefinitions/Decompositions/ResetWithoutReuse.qs

+    /// The qubit whose state is to be reset to $\ket{0}$.
+    @EnableTestingViaName("Test.TargetDefinitions.Reset")
+    operation Reset (target : Qubit) : Unit {
+        let r = M(target);


Since this doesn't necessarily leave the qubit in the |0> state -- which I assume is OK based on the file name -- it would be good to add a comment explaining this.

alan-geller · 2020-09-03T20:49:09Z

src/Simulation/TargetDefinitions/Decompositions/Utils.qs

+                    SpreadZ(qubits[1], qubits[2 .. Length(qubits) - 1]);
+                }
+                apply {
+                    ExpNoIdUtil([PauliZ,PauliZ], theta, [qubits[0], qubits[1]], rotation);


Why not just the within/apply here directly, since you know that Length(paulis) will be two and paulis[0] will be PauliZ? The recursion seems more confusing than clarifying in this case.

Agreed! I noticed this on a later pass in a different branch, but I'll go ahead and incorporate that change here.

alan-geller · 2020-09-03T20:52:25Z

src/Simulation/TargetDefinitions/Decompositions/Utils.qs

+        mutable denPow = denominatorPowerOfTwo;
+        while(num % 2 == 0) {
+            set num /= 2;
+            set denPow += 1;


Shouldn't that be -=? If you halve the numerator, you also want to halve the denominator, so you want to subtract 1 from the power in the denominator, I think.

Oh my, I think you are correct. It worries me that didn't cause any failures... Let me make that change and rerun the tests to confirm.

Actually, that change makes the tests fail, so we must not understand the convention of this helper. I'll put it back.

alan-geller · 2020-09-03T20:54:30Z

src/Simulation/TargetDefinitions/Decompositions/Utils.qs

+    }
+
+    @EnableTestingViaName("Test.TargetDefinitions.ArrayFromIndicesQ")
+    internal function ArrayFromIndiciesQ(values : Qubit[], indicies : Int[]) : Qubit[] {


Is there a reason you can't combine this function and the previous one into an ArrayFromIndices>'T>?

Also, it should be Indices, not Indicies.

Agreed, I can combine this into a single generic helper, which per suggestion from @cgranade I can model on Subarray for now, and replace with a call to that when/if the function gets moved into qsharp-runtime somewhere.

swernli · 2020-09-03T22:18:15Z

Why not Adj + Ctl? Exp is Adj+Ctl, so this could be, unless there's a reason for it not to be.

That's a good catch, thanks! The Ising gates are sort of special cases of a two qubit Exp rotation, so should be adjointable with the negative angle. I'll make sure to add that in!

cgranade · 2020-09-03T22:24:14Z

src/Simulation/TargetDefinitions/Decompositions/Utils.qs

+    }
+
+    @EnableTestingViaName("Test.TargetDefinitions.ArrayFromIndicesP")
+    internal function ArrayFromIndiciesP(values : Pauli[], indicies : Int[]) : Pauli[] {


This looks like Microsoft.Quantum.Arrays.Subarray from the standard library; should that be pulled back into runtime as well?

Good question! I'll defer to @cgranade .

Oh, that's a good idea! I don't think this change is the place to do it, but I do agree that as we find ourselves reimplementing useful helper functions from the library in the core or foundation packages we should seriously consider pulling the corresponding library function in.

swernli · 2020-09-04T04:34:10Z

Thanks for all the feedback and reviews!

swernli added 15 commits August 28, 2020 14:26

Remove deprecated diagnostics.

dce4209

Add Type2 targeting package

baafd3e

Clean up compilation and project references

7782a6e

Add Type2 to the solution

db77233

Separate out QsharpCore Simulator

93d6bea

Remove core simulator support for non-intrinsics

285ff24

Fixing core simulator packaging

af7361d

Type2 simulator and test infrastructure

0dcb195

Reduce test surface for Type2 to avoid duplicaiton

cced9b4

Adding implementation for new intrinsics

08b3bfc

Get input validation on decompositions working

9b8232b

Adding doc comments for checks.

960e1d1

Undoing rename to clean up diff

67ea6a7

Adding Type2 to packaging scripts

1141203

Clean up tests to minimize duplication

84a4c06

swernli requested review from ScottCarda-MS, alan-geller, bamarsha and bettinaheim August 28, 2020 21:28

swernli changed the title ~~swernli/decomp-type2~~ Adding Type2 targeting package and tests Aug 28, 2020

This was referenced Aug 28, 2020

Adding Type2 Targeting Package #353

Closed

Q# should include decompositions to alternate quantum gate sets #249

Closed

Fix manifest script

b103b63

ScottCarda-MS approved these changes Aug 28, 2020

View reviewed changes

ScottCarda-MS reviewed Aug 28, 2020

View reviewed changes

ScottCarda-MS self-requested a review August 28, 2020 22:39

swernli marked this pull request as draft August 28, 2020 22:45

swernli marked this pull request as ready for review August 28, 2020 22:45

Adding Ising gate descriptions

f87ecae

swernli added 3 commits September 2, 2020 00:53

Reworking sumulator package relationships

2559b13

Revert "Reworking sumulator package relationships"

b304c98

This reverts commit 2559b13.

Better refactor of simulator projects relationship

b731573

alan-geller approved these changes Sep 3, 2020

View reviewed changes

cgranade reviewed Sep 3, 2020

View reviewed changes

CR feedback and fixes

73bf72f

swernli merged commit 7632883 into feature/decomp Sep 4, 2020

swernli deleted the swernli/decomp-type2 branch September 4, 2020 05:10

	# identifies PackageReferences defined in the csproj, so all the dependencies
	# identify PackageReferences defined in the csproj, so all the dependencies

Adding Type2 targeting package and tests #363

Adding Type2 targeting package and tests #363

Uh oh!

Conversation

swernli commented Aug 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ScottCarda-MS left a comment

Choose a reason for hiding this comment

Uh oh!

swernli commented Aug 28, 2020

Uh oh!

swernli commented Aug 28, 2020

Uh oh!

ScottCarda-MS left a comment

Choose a reason for hiding this comment

Uh oh!

bamarsha commented Aug 28, 2020

Uh oh!

swernli commented Aug 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bamarsha commented Aug 28, 2020

Uh oh!

swernli commented Aug 30, 2020

Uh oh!

swernli commented Aug 31, 2020

Uh oh!

bamarsha commented Aug 31, 2020

Uh oh!

swernli commented Aug 31, 2020

Uh oh!

alan-geller commented Aug 31, 2020

Uh oh!

swernli commented Aug 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

swernli commented Sep 1, 2020

Uh oh!

swernli commented Sep 2, 2020

Uh oh!

swernli commented Sep 2, 2020

Uh oh!

alan-geller left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

swernli commented Aug 28, 2020 •

edited

Loading

swernli commented Aug 28, 2020 •

edited

Loading

swernli commented Aug 31, 2020 •

edited

Loading