Skip to content
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 BaseM #237

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Refactor BaseM #237

wants to merge 1 commit into from

Conversation

EarlMilktea
Copy link
Contributor

Context (if applicable):

Related to #178 .

Description of the change: (WIP)

  • Respect the original intention to introduce BaseM

@EarlMilktea EarlMilktea self-assigned this Nov 23, 2024
Copy link

codecov bot commented Nov 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.99%. Comparing base (7da2095) to head (70cd3d4).

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #237   +/-   ##
=======================================
  Coverage   78.99%   78.99%           
=======================================
  Files          40       40           
  Lines        6113     6114    +1     
=======================================
+ Hits         4829     4830    +1     
  Misses       1284     1284           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@EarlMilktea
Copy link
Contributor Author

@jemappellesami

I opened this PR to respect your initial motivation to introduce BaseM (add a way to simply specify the node subject to measure and nothing else), which is not properly reflected in the current code due to refactoring in process.

In addition to that, I have the following suggestion:

  • Remove MeasureMethod and related things

I feel it's clearer to use BaseM (or something derived from this) to specify everything about measurements, as the data required to perform measurement are essentially the same (angle, plane) though they can be missing at first.
I would like to know in detail how you are trying to utilize the simulators without specifying the details, even while the simulators are initially designed for non-blind measurements.

@thierry-martinez
Copy link
Contributor

LGTM! Thanks.

The purpose of BaseM is to express blind patterns, meaning patterns where the server or simulator does not have access to the specification of the measurement. In this type of setting, it is the responsibility of MeasureMethod—provided by the client—to supply the measurement details missing in BaseM. However, for the security protocol to remain correct, these details must be computed dynamically during the simulation; they cannot be hardcoded into the pattern itself.

Comment on lines +54 to +58
class BaseM:
"""Base class for measurement command.

This class does not contain any data except for the node to measure.
"""
Copy link
Contributor

@thierry-martinez thierry-martinez Nov 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation added in commit ad18270 and later deleted in #205 was more informative. I believe we should revert to that version.

Suggested change
class BaseM:
"""Base class for measurement command.
This class does not contain any data except for the node to measure.
"""
class BaseM:
"""Base measurement command.
Represent a measurement of a node. In MBQC, a measure is an instance of `M`,
with given plane, angles, and domains. In the context of blind computations,
the server only knows which node is measured, and the parameters are given
by the :class:`graphix.simulator.MeasureMethod` provided by the client.
"""

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I intentionally trimmed them to ensure that no outdated info. is left.
Of course willing to revert to the original after discussion.

@mgarnier59
Copy link
Contributor

mgarnier59 commented Nov 27, 2024

As far as I remember we didn't agree on anything regarding measurement commands. Was it even raised as an issue? The structure BaseMand MeasureMethod has been done on purpose for our needs and we heavily rely on this so please don't modify anything without thorough discussion beforehand.

Furthermore, your PR message

Respect the original intention to introduce BaseM

is cryptic and it would be best to be more precise in the future.

@EarlMilktea
Copy link
Contributor Author

The structure BaseM and MeasureMethod has been done on purpose for our needs

I opened this PR to improve this structure itself before aggressively refactoring simulators.
I know we need to care about backward compatibility, but I never believe that we can implement efficient, usable, and typed simulators without introducing breaking changes.
Additionally I would like to emphasize that, though I created it without discussion I never intend to merge it without discussion: that's why I'm using draft PR.
Please view this PR as a developer-friendly issue (can suggest by example) or something.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants