Broken up QuantumSimulator into CommonNativeSimulator and QuantumSimulator #853
Conversation
src/Simulation/Simulators/CommonNativeSimulator/CommonNativeSimulator.cs
Show resolved
Hide resolved
src/Simulation/Simulators/CommonNativeSimulator/CommonNativeSimulator.cs
Show resolved
Hide resolved
src/Simulation/Simulators/CommonNativeSimulator/QubitManager.cs
Outdated
Show resolved
Hide resolved
cgranade
left a comment
cgranade
left a comment
There was a problem hiding this comment.
I'm somewhat concerned about using the outdated implementation of Dump from QuantumSimulator for new code, as that will make exposing the sparse simulator to Python and Q# notebook users much more difficult. We should make sure that these changes are consistent with allowing the sparse simulator to be used by notebook and Python users; please let me know if I can help figure out a path to do so. Thanks!
| /// it attempts to create the wave function or the resulting subsystem; if it fails | ||
| /// because the qubits are entangled with some external qubit, it just generates a message. | ||
| /// </summary> | ||
| protected virtual QVoid Dump<T>(T target, IQArray<Qubit>? qubits = null) |
There was a problem hiding this comment.
This may be a good opportunity to simplify Dump and make it work more nicely with the diagnostics events in SimulatorBase. At the moment IQ# needs to handle QuantumSimulator.Dump differently than for any other simulator due to how it uses Dump in a very different way; the sparse simulator really shouldn't use the same implementation as QuantumSimulator but should use the newer events in SimulatorBase.
There was a problem hiding this comment.
I assume that the following words in this CR note also relate to this Dimp():
I'm somewhat concerned about using the outdated implementation of Dump from QuantumSimulator for new code, as that will make exposing the sparse simulator to Python and Q# notebook users much more difficult. We should make sure that these changes are consistent with allowing the sparse simulator to be used by notebook and Python users; please let me know if I can help figure out a path to do so. Thanks!
Please feel free to provide more details about this in the issue #855. I'm not very familiar with the plethora of simulators at the moment. Any details are welcome.
I plan to consider the simplification of Dump() and unifying it with other simulators' implementation separately from this PR.
There was a problem hiding this comment.
Happy to discuss in more detail on a call if that would be more helpful, but currently the only simulator that needs special hacks in IQ# in order to expose diagnostics to Python callers is QuantumSimulator. I'd actually consider that to be a blocking issue for this PR, since these changes would mean that IQ# also needs to use one-off hacks to support diagnostics in the sparse simulator.
There was a problem hiding this comment.
Discussed offline. We agreed that the simplification and unification of the Dump() is not a blocking issue for this PR but may be a blocking issue for subsequent refactoring of the SparseSimulator to become a derived class of the CommonNativeSimulator.
bettinaheim
left a comment
bettinaheim
left a comment
There was a problem hiding this comment.
I think we might need to keep passing a simulator id to operation calls to be able to avoid threading issues. @swernli, does that ring a bell by any chance?
src/Simulation/Simulators/CommonNativeSimulator/QubitManager.cs
Outdated
Show resolved
Hide resolved
|
Please review. |
|
PLease review |
| namespace Microsoft.Quantum.Simulation.Simulators | ||
| { | ||
| public partial class QuantumSimulator | ||
| public partial class CommonNativeSimulator |
There was a problem hiding this comment.
Would something like NativeSimulatorBase be a better name? Other simulators seem to inherit from it.
4 participants
Have broken up QuantumSimulator (the C# wrapper around Native full-state simulator) into
CommonNativeSimulator (common part for QuantumSimulator and SparseSimulator)
and QuantumSimulator (full-state-simulator-specific part).