Sparse simulator by Samuel Jaques #835
Conversation
Addressing sparse simulator feedback
…ck-2 More fixes from feedback
…ck-3 Unit tests, comments and feedback from review
…ck-3 Some build fixes
Iterator dereference fix
DmitryVasilevsky
requested review from
bettinaheim,
kuzminrobin,
swernli and
thomashaener
thomashaener
left a comment
thomashaener
left a comment
There was a problem hiding this comment.
Thanks! I've added a few suggestions & comments.
src/Simulation/Simulators/SparseSimulator/Native/SparseSimulator.h
Outdated
Show resolved
Hide resolved
src/Simulation/Simulators/SparseSimulator/Native/SparseSimulator.h
Outdated
Show resolved
Hide resolved
|
|
||
|
|
||
| ~SparseSimulator() { | ||
| _execute_queued_ops(); |
There was a problem hiding this comment.
Is this really necessary? If so, should make sure it doesn't throw.
There was a problem hiding this comment.
It actually does throw right now as asserts may be queued...
src/Simulation/Simulators/SparseSimulator/Native/quantum_state.hpp
Outdated
Show resolved
Hide resolved
src/Simulation/Simulators/SparseSimulator/Native/quantum_state.hpp
Outdated
Show resolved
Hide resolved
src/Simulation/Simulators/SparseSimulator/Native/quantum_state.hpp
Outdated
Show resolved
Hide resolved
src/Simulation/Simulators/SparseSimulator/Native/quantum_state.hpp
Outdated
Show resolved
Hide resolved
src/Simulation/Simulators/SparseSimulator/Native/quantum_state.hpp
Outdated
Show resolved
Hide resolved
kuzminrobin
left a comment
kuzminrobin
left a comment
There was a problem hiding this comment.
As I understand this branch is just the latest state of the sam-jaques' PR. If that's the case then approving.
We just need to make sure this PR builds. After merging we can address all the feedback in our qsharp-runtime repo.
Co-authored-by: Thomas Haener <[email protected]>
Yes, this is the plan. We need it building. I will also address Thomas' feedback here. |
src/Simulation/Simulators/SparseSimulator/Native/basic_quantum_state.hpp
Show resolved
Hide resolved
|
Had a few more comments that would be good to handle before merging to |
| namespace Microsoft::Quantum::SPARSESIMULATOR | ||
| { |
There was a problem hiding this comment.
[nit] This file seems to use a mix of brace-on-same and brace-on-next code styles?
| { | ||
| var result = Marshal.PtrToStringAnsi(Sample_cpp(Id)).ToCharArray(); | ||
| Array.Reverse(result); | ||
| var bool_result = new bool[register.Length]; |
There was a problem hiding this comment.
bool_result should be boolResult in C# style.
| var bool_result = new bool[register.Length]; | ||
| for (int i=0; i < register.Length; i++) | ||
| { | ||
| bool_result[i] = (result[register[i].Id] == '1' ? true : false); |
There was a problem hiding this comment.
You could probably use something like LINQ to write 348–353 out a bit easier:
return register.Select(qubit => result[qubit.Id] == '1' ? true : false).ToArray();|
|
||
| // Returns the amplitude of a specific state, given by a string representing its label | ||
| [DllImport(simulator_dll)] | ||
| private static extern void GetAmplitude_cpp(uint sim, uint label_length, char[] label, ref double real, ref double imag); |
There was a problem hiding this comment.
Should these be ref or out?
| { | ||
| double real = 0; | ||
| double imag = 0; | ||
| GetAmplitude_cpp(Id, (uint)label.Length, label.ToCharArray(), ref real, ref imag); |
There was a problem hiding this comment.
If you define the GetAmplitude_cpp method with real and imag as out instead of ref, this wrapper method gets a bit easier:
GetAmplitude_cpp(Id, (uint)label.Length, length.ToCharArray(), out var real, out var image);
return new Microsoft.Quantum.Math.Complex((real, imag));|
|
||
| // It's expensive for the C++ simulator to separate two parts of a wavefunction, | ||
| // so if we just want the full state, it calls a different function | ||
| if (qubits == null) |
There was a problem hiding this comment.
What if qubits is not null, but consists of all currently allocated qubits?
|
@DmitryVasilevsky, in order to fix the build can you please apply the following change? This change excludes SparseSimulator form (I fail to push my change branch because I don't have access to Sam's repo, I didn't accept the permission form sam-jaques on time, and his permission has expired, so I requested again) diff --git a/src/Simulation/Simulators/Microsoft.Quantum.Simulators.csproj b/src/Simulation/Simulators/Microsoft.Quantum.Simulators.csproj
index 73e63818..0869cee1 100644
--- a/src/Simulation/Simulators/Microsoft.Quantum.Simulators.csproj
+++ b/src/Simulation/Simulators/Microsoft.Quantum.Simulators.csproj
@@ -46,4 +46,11 @@
</None>
</ItemGroup>
+ <ItemGroup>
+ <BondCodegen Remove="SparseSimulator\**" />
+ <Compile Remove="SparseSimulator\**" />
+ <EmbeddedResource Remove="SparseSimulator\**" />
+ <QSharpCompile Remove="SparseSimulator\**" />
+ </ItemGroup>
+
</Project> |
@cgranade : This isn't going to main yet. This goes into a branch in our repo (from external contributor). We need to pull this code first and then fix it. I will address your comments after we pull the code but before it goes to main. |
6 participants
Samuel Jaques contributed this sparse simulator code.
After removing some experimental code and unnecessary external components, this branch is ready to be pulled into a branch in qsharp-runtime repo.
Sparse simulator tests pass.
Once code is pulled into a branch in our repo, we will address the rest of the feedback.
Specifically some good feedback is still to be addressed from the initial PR: #767
PR will be merged without addressing entire feedback:
Some feedback is addressed, but the rest of the feedback will be addressed before merging to main.
It is much more convenient to address it in our repository.