Refactored the Dump functionality on Q#RT side. #898
Conversation
|
/azp help |
Supported commands
See additional documentation. |
kuzminrobin
requested review from
DmitryVasilevsky,
bettinaheim,
cgranade and
swernli
kuzminrobin
force-pushed
the
feature/simplifyDump3
branch
from
45b255e to
1fecf7f
Compare
cgranade
left a comment
cgranade
left a comment
There was a problem hiding this comment.
Looks good to me! Had a last few comments, mostly about duplicated functionality in the string output for displayable states, but I think this is getting pretty close. Please let me know how I can help!
src/Simulation/Simulators/CommonNativeSimulator/CommonNativeSimulator.cs
Outdated
Show resolved
Hide resolved
| /// <see cref="Microsoft.Quantum.Simulation.Simulators.CommonNativeSimulator.StateDumper.Dump" />. | ||
| /// </remarks> | ||
| [JsonProperty("amplitudes")] | ||
| public Complex[]? Amplitudes { get; set; } |
There was a problem hiding this comment.
No suggested action at the moment, but when adding the sparse simulator, this will need to be generalized to a dictionary from indices to amplitudes.
There was a problem hiding this comment.
Changed to a dictionary.
| _count = qubits == null | ||
| ? this.Simulator.QubitManager.AllocatedQubitsCount | ||
| : qubits.Length; | ||
| _data = new Complex[1 << ((int)_count)]; // If 0 qubits are allocated then the array has |
There was a problem hiding this comment.
No suggested action at the moment, but this will likely need to be changed to a dictionary ass well once the sparse simulator is added.
There was a problem hiding this comment.
Changed to a dictionary.
cgranade
left a comment
cgranade
left a comment
There was a problem hiding this comment.
Sorry, accidentally hit comment instead of approve.
cgranade
left a comment
cgranade
left a comment
There was a problem hiding this comment.
Ah, you went on and modified DisplayableState to use a dictionary already! That should make adding in the sparse simulator much easier — your most recent changes should be precisely the logic needed to handle an incomplete sequence of callback calls.
Left a couple last comments about adapting some of SignificantAmplitudes to the new property, but otherwise looks good to me.
| ( | ||
| truncateSmallAmplitudes | ||
| ? Amplitudes | ||
| .Select(idx_amplitude => idx_amplitude) |
There was a problem hiding this comment.
I think this line may not be needed now that Amplitudes is a dictionary?
| .Select(idx_amplitude => idx_amplitude) |
In any case, idx_amplitude is a KeyValuePair with the new definition of Amplitudes, such that it's no longer an index:
[nit / code style: the name idx_amplitude is in snake_case but should be in camelCase.]
| .Where(item => | ||
| System.Math.Pow(item.Value.Magnitude, 2.0) >= truncationThreshold | ||
| ) | ||
| : Amplitudes.Select(idx_amplitude => idx_amplitude) |
There was a problem hiding this comment.
Similarly here, the call to Select that was previously used to attach indices shouldn't be needed any more:
| : Amplitudes.Select(idx_amplitude => idx_amplitude) | |
| : Amplitudes |
kuzminrobin
force-pushed
the
feature/simplifyDump3
branch
from
5ba53b0 to
065d3ad
Compare
Simplified the Dump functionality for QuantumSimulator and the functionality use in IQSharp.
This is a part of the set of 3 PRs:
Q#RT side,
IQ#,
Katas.