-
Notifications
You must be signed in to change notification settings - Fork 21
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor backend for delegated measurement methods #178
Conversation
Indeed, the attribute Statevec.Nqubit leads to problems that have to be fixed
some test fixed, some other not (related to DM and TN backend)
This commit allows the user to pass a `graphix.sim.backend.Backend` instance to `graphix.simulator.PatternSimulator` (formerly, only the strings `"statevector"`, `"densitymatrix"`, and `"tensornetwork"` were accepted as `backend`, and the simulator constructed the backend accordingly). The constructor of `graphix.sim.base_backend.Backend` now accepts an optional keyword argument `meas_op` of type `graphix.sim.base_backend.MeasureMethod`. By default, the class `graphix.sim.base_backend.DefaultMeasureMethod` is used and implements MBQC. Other protocols such as VBQC can be implemented by constructing the backend with another measurement operator.
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.
My aplogies for the delay reviewing this!
def set_measure_result(self, node: int, result: bool) -> None: ... | ||
|
||
|
||
class DefaultMeasureMethod(MeasureMethod): |
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.
Could you add description of this class, esp. why we need to have this in the first place? I believe this is for generalization (e.g. client-server setup), but it is hard to understand why we have this without such context, as this looks (at first sight) a rather unnecessary wrapping of measurement command.
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.
It is exactly for that reason (see 4th point in the description of the PR): this class is to be overwritten by a custom measurement method in the case of a client-server setup. You are right we should add a comment about it in the code.
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! this kind of targeted API change would require a simple but complete example demonstrating such use and/or a dedicated tutorial page. could you write up an example?
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.
We have written an example in our repo https://github.com/qat-inria/veriphix as mentioned in the PR description. We now added a reference to this example in in the docstring of the MeasureMethod
class
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! tbh, I feel that delegating the explanation and justification of implementation decisions to the package that depends on graphix is a little strange. it is like rewriting some part of numpy for the sake of graphix and justifying by linking graphix example. I would like this repo to be standalone to be maintainable, and I would appreciate it if you could add short example in graphix package without relying on verphix if at all possible.
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.
@jemappellesami @thierry-martinez I guess I'll write one up myself..! let us merge this now and we can get on with other PRs!
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.
General comment:
- Please do not omit type annotation, no matter how it is used.
- Please verify that you're surely importing the name up to the required layer: I'm seeing many linter warnings originating from
foo.bar.baz
used withoutimport foo.bar
(onlyimport foo
). This is prohibited unlessbar
is imported in__init__.py
offoo
.
Co-authored-by: S.S. <[email protected]>
Co-authored-by: S.S. <[email protected]>
Co-authored-by: S.S. <[email protected]>
Co-authored-by: S.S. <[email protected]>
@thierry-martinez @EarlMilktea thanks for nice in-depth discussions. let us resolve the remaining two comments, and merge this soon! |
Discussion with Shinichi and EarlMilktea here: TeamGraphix#178 (comment)
Suggested by EarlMilktea: TeamGraphix#178 (comment)
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.
@thierry-martinez please resolve conflicts as appropriate and squash and merge. Thanks!
Merged. Thanks! |
Description of the change:
Refactoring of the backend to enable delegated measurement-based QC protocols.
NodeIndex
data structure for efficient mapping between nodes and indices in the backend's internal stateMeasureMethod
, with default measurement method that implements MBQC. It allows customized measurement methods, for instance for delegated QC protocols.See examples of usage of these features for verification of delegated MBQC quantum computations in https://github.com/qat-inria/veriphix .