Preparing the QuantumSimulator for unifying the Dump(). by kuzminrobin · Pull Request #867 · microsoft/qsharp-runtime

kuzminrobin · 2021-11-10T02:07:50Z

This PR is a stage one of the three below:

Preparing the QuantumSimulator for unifying the Dump(). Providing the new dump functionality on the qsharp-runtime repo side.
Redirecting the IQSharp repo to the new dump functionality in qsharp-runtime repo.
Cleaning-up the commented out code on the qsharp-runtime repo side.

cgranade

This looks like a really good start, thanks for tagging me in on this! There's some minor suggestions that I could make as well as call forwards to an eventual IQ# PR, but I think this should be great for unblocking progress on the IQ# side of the refactoring.

src/Simulation/Simulators/QuantumSimulator/Dump.cs

src/Simulation/Simulators/QuantumSimulator/StateDumper.cs

cgranade · 2021-11-10T17:28:58Z

src/Simulation/Simulators/QuantumSimulator/StateDumper.cs

+            ///     will be displayed.
+            /// </summary>
+            [JsonProperty("div_id")]
+            public string DivId { get; set; } = string.Empty;


We didn't get a chance to discuss this property offline, sorry, but we may want to find a way to move this property over to IQ# as well; having the div id here couples the runtime to our HTML-based visualization infrastructure.

(My familiarity with this functionality is just intuitive)
May I know your suggestion?
Is there anything related to this topic that I need to address in this PR? (anything that will prevent the next PR on IQ# side to run smoothly in one attempt/step)
I'm happy to discuss this offline, feel free to give me a call whenever I'm available.

My initial thought would be to drop this entirely from the definition of the state, and to refactor the IQ# side to use the more general DisplayableHtmlElement type in that repo to wrap a state in a div. Less work, but another possibility would be to replace the div id here by just a generic System.Guid that can be used to uniquely refer to the state later.

(FYI: I'm not familiar with DisplayableHtmlElement, IQ# code base in general, very fresh to C#. At the moment it is easier for me to follow particular instructions about what and how to change)

For now I'm commenting out all the fragments related to DivId with a note to get back to this when refactoring IQ# side:

[JsonProperty("div_id")] public string DivId { get; set; } = string.Empty; . . . var id = System.Guid.NewGuid(); // FYI: System.Guid is already used here. var state = new DisplayableState { . . . DivId = $"dump-machine-div-{id}" };

src/Simulation/Simulators/QuantumSimulator/StateDumper.cs

cgranade · 2021-11-10T17:34:33Z

src/Simulation/Simulators/QuantumSimulator/StateDumper.cs

            /// </summary>
-            public virtual string FormatBaseState(uint idx) =>
-                $"∣{idx.ToString().PadLeft(_maxCharsStateId, ' ')}❭";
+            public virtual string FormatBaseState(uint idx) 


This method is now redundant with the more general logic that you moved above; in particular, this method is equivalent to BasisStateLabel(BasisStateLabelingConvention.LittleEndian, idx). Since it's public we may not be able to remove it, but it'd be good to at least make this method obsolete so that we can give a path for users to use conventions other than little endian from Q# standalone programs or Q# programs called from Q#, as well as from Q# notebooks or Q# + Python programs.

FYI:
The diff of this file does not display the change properly.

The whole SimpleDumper with its Format..() logic (consisting of

string Format(uint idx, double real, double img)

string FormatBaseState(uint idx)

string FormatCartesian(double real, double img)

string FormatPolar(double magnitude, double angle)

string FormatMagnitude(double magnitude, double phase)

string FormatAngle(double magnitude, double angle)

) has been removed.
The enum BasisStateLabelingConvention and class DisplayableState have been added. The Format..() logic has been moved from the removed SimpleDumper to the class DisplayableState. In that Format..() logic the FormatBaseState() has been updated slightly. That is why in that Format..() logic only the FormatBaseState() is shown (by git diff) as new, but in reality the whole Format..() logic is new in this class DisplayableState.

I assume that with your words
"Since it's public we may not be able to remove it" you mean
"Since the FormatBaseState() is public we may not be able to remove it".
I'd like to admit that the FormatBaseState() is totally new for class DisplayableState, that is why we can remove it.

I was also thinking may be I should encapsulate this Format..() logic into a class (probably defined inside of DisplayableState) to keep the Format..() logic "as a single piece". In which case it would make sense to preserve the FormatBaseState().
Or I can remove the FormatBaseState() and replace the call to it with BasisStateLabel(BasisStateLabelingConvention.LittleEndian, idx).

What would you recommend?

src/Simulation/Simulators/QuantumSimulator/StateDumper.cs

cgranade · 2021-11-10T18:19:02Z

src/Simulation/Simulators/QuantumSimulator/StateDumper.cs


            }

+            public override string ToString()


For the eventual IQ# PR, we may want to refactor its plain-text display encoder to use the new implementation here so that it can reuse FormatCartesian and FormatPolar instead of re-implementing. If we do that, it may be worth making ToString call a new overload of ToString that allows passing more options. That could likely be deferred until we have the matching IQ# PR closer to being ready to merge in.

I assume you mean something like this:

public string ToString(BasisStateLabelingConvention convention, // Non-override. Parameterized. bool truncateSmallAmplitudes, double truncationThreshold) { return string.Join('\n', SignificantAmplitudes(convention, truncateSmallAmplitudes, truncationThreshold) .Select( item => { var (cmplx, basisLabel) = item; var amplitude = (cmplx.Real * cmplx.Real) + (cmplx.Imaginary * cmplx.Imaginary); var angle = System.Math.Atan2(cmplx.Imaginary, cmplx.Real); return $"|{basisLabel}⟩\t{FormatCartesian(cmplx.Real, cmplx.Imaginary)}\t == \t" + $"{FormatPolar(amplitude, angle)}"; })); } public override string ToString() // An override of the `object.ToString()`. { return ToString(BasisStateLabelingConvention.LittleEndian, true, 0.8); // Fixed values!!! }

Would you recommend to make this change now in this PR?

As mentioned, I don't think it's urgent to include in this PR. At the same there's also no harm in doing so.

var amplitude = (cmplx.Real * cmplx.Real) + (cmplx.Imaginary * cmplx.Imaginary); var angle = System.Math.Atan2(cmplx.Imaginary, cmplx.Real);

As a side note, I believe that System.Numerics.Complex already has properties for that, such that the Select expression can be significantly simplified.

public override string ToString() => // An override of the `object.ToString()`. ToString(BasisStateLabelingConvention.LittleEndian, false, 0.0); // Fixed values!!!

Setting truncation to false should reproduce previous behavior; the threshold is ignored in that case.

As a side note, I believe that System.Numerics.Complex already has properties for that, such that the Select expression can be significantly simplified.

Based on System.Numerics Methods, here is what I've come up with, not sure if it is absolutely right (already forgot a lot of Complex arithmetic):

var amplitude = Complex.Abs(cmplx) * Complex.Abs(cmplx); // (cmplx.Real * cmplx.Real) + (cmplx.Imaginary * cmplx.Imaginary); var angle = Complex.Atan(cmplx); // System.Math.Atan2(cmplx.Imaginary, cmplx.Real);

amplitude can be replaced by the cmplx.Amplitude property and angle can be replaced by Amplitude.Phase.

FYI (I have just noticed your last note in this discussion):

var amplitude = cmplx.Amplitude;

Compiler error:
'Complex' does not contain a definition for 'Amplitude' and no accessible extension method 'Amplitude' accepting a first argument of type 'Complex' could be found (are you missing a using directive or an assembly reference?) [Microsoft.Quantum.Simulators]csharp(CS1061)

I also fail to find "Amplitude" in Complex, its Properties, etc.

Sorry, I should have written Magnitude instead of Amplitude.

Ah, now I understand. Thank you.
From the very beginning, prior to my change, the line

var amplitude = (real * real) + (img * img);

should have been

var magnitude = Math.Sqrt(real * real + img * img); // `Math.Sqrt()` was missing, and named magitude.

I'll simplify during stage 3.

DmitryVasilevsky · 2021-11-11T03:22:27Z

src/Simulation/Simulators/QuantumSimulator/StateDumper.cs

+        ///     given their representations as strings of classical bits.
        /// </summary>
-        public class SimpleDumper : StateDumper
+        [JsonConverter(typeof(StringEnumConverter))]


Can you explain the meaning of this in a comment?

This code has been moved from IQSharp repo.
@cgranade, can you tell what that fragment means?

This fragment is a fairly standard JSON serialization attribute that specifies enum values are converted as strings, not as ints.

DmitryVasilevsky · 2021-11-11T03:28:35Z

src/Simulation/Simulators/QuantumSimulator/StateDumper.cs

+                    // numerically rather than lexographically.
+                    convention switch {
+                        BasisStateLabelingConvention.BigEndian => ToIntComparer,
+                        BasisStateLabelingConvention.LittleEndian => ToIntComparer,


Would be nice to explain why both conventions result in the same comparer.

This code has been moved from IQSharp repo too.
Based on the comments I assume that for numerical values (regardless of the endianness) the numerical comparer is used, but for the string values - the lexicographical comparer.
(@cgranade, please correct me if I'm wrong)

Big-endian and little-endian are both used as different conventions for producing an int from a bitstring, but once you have an int, natural ordering on integers applies from there in both cases.

And having this in a comment would be nice.

I'm leaving this and a few more items as non-resolved. I will address those at stage 3 (see PR description).

DmitryVasilevsky · 2021-11-11T03:35:34Z

src/Simulation/Simulators/QuantumSimulator/StateDumper.cs

+                    BasisStateLabelingConvention.BigEndian =>
+                        System.Convert.ToInt64(
+                            String.Concat(
+                                System.Convert.ToString(index, 2).PadLeft(NQubits, '0').Reverse()


The case above should probably also be written in one line.

Makes sense. If I'll be making more changes (with subsequent CI rebuild) then I'll make this change.
But if the build succeeds then I'd prefer to merge.

This reverts commit b30286c.

…" (#873)

kuzminrobin self-assigned this Nov 10, 2021

cgranade reviewed Nov 10, 2021

View reviewed changes

kuzminrobin added 7 commits November 10, 2021 16:43

Minor clarifying comments

4101d38

Moved some classes from IQSharp

3c7c967

Minor clean-up.

5d1ea5c

Added DisaplyableState.ToString() wiht formatting

7cd3b0b

Minor clean-up

5d69a45

CR changes

64d157c

Minor clean-up

828935e

kuzminrobin force-pushed the kuzminrobin/simplifyDump2 branch from 29cd940 to 828935e Compare November 11, 2021 00:43

kuzminrobin requested review from DmitryVasilevsky, bettinaheim and cgranade November 11, 2021 00:55

kuzminrobin marked this pull request as ready for review November 11, 2021 00:57

DmitryVasilevsky reviewed Nov 11, 2021

View reviewed changes

DmitryVasilevsky approved these changes Nov 11, 2021

View reviewed changes

kuzminrobin enabled auto-merge (squash) November 11, 2021 04:30

kuzminrobin merged commit b30286c into main Nov 11, 2021

kuzminrobin deleted the kuzminrobin/simplifyDump2 branch November 11, 2021 18:02

bettinaheim pushed a commit that referenced this pull request Nov 17, 2021

Revert "Preparing the QuantumSimulator for unifying the Dump(). (#867)"

375c626

This reverts commit b30286c.

bettinaheim added a commit that referenced this pull request Nov 18, 2021

Revert "Preparing the QuantumSimulator for unifying the Dump(). (#867)…

0f66dfe

…" (#873)

kuzminrobin restored the kuzminrobin/simplifyDump2 branch January 11, 2022 23:11

kuzminrobin deleted the kuzminrobin/simplifyDump2 branch January 14, 2022 23:56

Conversation

kuzminrobin commented Nov 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cgranade left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kuzminrobin Nov 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kuzminrobin Nov 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

kuzminrobin Nov 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kuzminrobin commented Nov 10, 2021 •

edited

Loading

kuzminrobin Nov 10, 2021 •

edited

Loading

kuzminrobin Nov 10, 2021 •

edited

Loading

kuzminrobin Nov 10, 2021 •

edited

Loading