Improvements to M.Q.Diagnostics namespace #302
Improvements to M.Q.Diagnostics namespace #302
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just left some comments for the current draft PR.
Co-authored-by: Mathias Soeken <mathias.soeken@outlook.com>
Looks like the test process crashed, will rerun on the next commit. |
Co-authored-by: Mathias Soeken <mathias.soeken@outlook.com>
Co-authored-by: Mathias Soeken <mathias.soeken@outlook.com>
@anpaz-msft: It looks like the Linux test host is crashing pretty frequently again, would you have any suggestions? Thanks!
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looked primarily at the visualization code here; looks good to me! Will allow other domain experts to give their approval on the PR.
Visualization/src/Extensions.cs
Outdated
// TODO: remove once https://github.com/microsoft/iqsharp/pull/239 is | ||
// resolved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a reminder about this TODO
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching that, @rmshaffer, I did indeed forget to update after microsoft/iqsharp#239 completed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made a pass and left some comments, mostly about documentation, tests and examples. Looks very promising! We might be able to retire parts of CounterSimulator, if not all of it.
I'm assuming that DumpOperation works in command line Q# as well as in Jupyter notebooks, right? I'd love to finally update DumpUnitary to dump the real unitary and not the column-by-column approximation!
}); | ||
failed = true; | ||
throw new ExecutionFailException( | ||
$"Operation {op.FullName} was called more than the allowed {nTimes} times." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this exception upon calling Adjoint AllowAtMostNCallsCA
, I'd suggest adding the number of times the operation was called to the message; it can be helpful to troubleshooting if you know by how much exactly you overuse the operation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also add a special message for nTimes = 0
- it's easier to read something like "You are not allowed to call ... in this code" than "more than the allowed 0 times"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this exception upon calling
Adjoint AllowAtMostNCallsCA
, I'd suggest adding the number of times the operation was called to the message; it can be helpful to troubleshooting if you know by how much exactly you overuse the operation.
It fails on the (n +1)st call, as soon as the required condition fails; I agree, it may make sense to defer until the end of the allow-block, though.
|
||
namespace Microsoft.Quantum.Diagnostics | ||
{ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to add a functionality to get the number of calls of the specified operation? The context is similar to this one - sometimes I want to check that at most X calls to a group of similar operations have been done, but it could be one or another. I don't quite see how to get this functionality with the existing implementation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, there's no way to do exactly that using this feature as currently written. Given the short timescale until the next release, it may make sense to defer that as a feature request for September?
Thanks for the feedback @tcNickolas, I appreciate it!
Given your comments, it looks like there may be some usecases still left (e.g.: enforcing constraints on groups of operations), but yeah; my goal was to be able to consolidate that logic in a simulator-independent fashion, so that oracle counting policies could be applied against the Toffoli simulator, or even against third-party simulators.
That should be the case, yes. In particular, the For Q# standalone programs, I'm not sure if that event is wired up automatically. It looks like https://github.com/microsoft/qsharp-runtime/blob/master/src/Simulation/CsharpGeneration/EntryPoint.fs#L120 may need modified to add a listener to the In any case, I've added a
That's one of the big advantages of using the Choi–Jamiołkowski isomorphism (also known in this case as AAPT) to find unitary representations; any relative phases between the various states in an input basis are directly captured by the use of a reference register, so that only phases that are truly global factor out. Still happy to share more about that if you're interested some time. In the meantime, I've made a pass on your feedback and will update the PR accordingly. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just left one small comment. Thanks
Dismissing stale review. Remaining comments deferred to #312.
Thank you @cgranade, this is very nice addition to the diagnostics namespace. It looks like there could be situations where |
Thanks for the kind words! Agreed, different simulators will likely have different formats for dumping operators; just as https://github.com/qsharp-community/chp-sim overrides DumpRegister and DumpMachine to show stabilizer tableaus, I'd love to see that project outputting Cliffords as tables of Pauli mappings, similar to what http://www.cgranade.com/python-quaec/ does. Similarly, I'd love to hook up ToffoliSimulator to output truth tables. From that perspective, the design of this feature is intended to allow simulators defined downstream from Microsoft.Quantum.Standard to override DumpOperation, and for the DumpOperation emulation class add support for simulators defined upstream from Microsoft.Quantum.Standard. |
This PR (still in draft for early feedback and API review) provides a number of improvements to the Microsoft.Quantum.Diagnostics namespace in the Microsoft.Quantum.Standard package:
This PR provides a new package, Microsoft.Quantum.Standard.Visualization, that uses the runtime emulation feature to interpret diagnostics sent to the
MaybeDisplayDiagnostic
event, allowing for improved diagnostics support in IQ#. This architecture reduces cross-repo dependencies, as visualizations can be provided in library runtime emulation classes, without requiring modifications to the runtime or to IQ#.DumpOperation
callableThis PR provides a new operation,
DumpOperation
, that uses the Choi–Jamiłkowski isomorphism andDumpRegister
together to extract a unitary representation of adjointable operations and to display this representation as a diagnostic. On machines that do not support this representation, but that support theH
andCNOT
intrinsics andDumpMachine
, the Choi–Jamiłkowski state for the given operation is shown instead. On machines that support neither, the operation can be safely ignored.Allow*
callablesThis PR also provides operations using a new verb,
Allow*
, representing that a condition must hold inside of theapply
arm of awithin
/apply
block. For example, this functionality can in principle be used in some oracle-based katas by enforcing that a given oracle operation is not called more than once within a givenapply
. Similarly,AllowAtMostNQubits
fails when more than a given number of qubits is allocated inside of anapply
block.Remaining work:
NB: unit testing may require the legacy Q# unit testing framework, so as to allow for
ShouldFail
tests.+@tcNickolas for possible application to oracle-based katas, +@rmshaffer for IQ# impact.
+@guenp and @msoeken for API review.