-
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
Fix #45: Parameterized measurement patterns #158
base: master
Are you sure you want to change the base?
Fix #45: Parameterized measurement patterns #158
Conversation
3093bdd
to
a5e16de
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #158 +/- ##
==========================================
+ Coverage 71.82% 72.95% +1.13%
==========================================
Files 30 31 +1
Lines 5359 6038 +679
==========================================
+ Hits 3849 4405 +556
- Misses 1510 1633 +123 ☔ View full report in Codecov by Sentry. |
@thierry-martinez great, is the intention to merge @pafloxy's #68-equivalent code first (when it's ready), before merging this? |
I adapted #68 code for The test |
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 think as for now this is all that I was looking for, If @shinich1 thinks its alright maybe we should go for the merge. :)
@thierry-martinez @pafloxy great! give us a few days to look through. A few quick comments:
|
Let me review it later. Anyway, is it absolutely necessary to use sympy?
Additionally, let me point out that @masa10-f is planning to completely eliminate sympy from the source. |
If sympy is absolutely necessary, it's OK, but we may need to verify that we're using it wisely. |
graphix/parameter.py
Outdated
expected by the simulator back-ends. | ||
""" | ||
|
||
def __init__(self, expression: sp.Expr): |
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.
Please do not omit -> None
.
graphix/parameter.py
Outdated
def __repr__(self) -> str: | ||
return str(self._expression) | ||
|
||
def subs(self, variable, value) -> ParameterExpression | numbers.Number: |
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.
Please use complex
instead of abstract numbers.Number
.
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.
Numbers from numpy
(or sympy
...) are not necessarily instances of complex
, for instance:
isinstance(np.array([1])[0], complex) == False
isinstance(np.array([1])[0], numbers.Number) == True
isinstance(sp.parse_expr(1), complex) == False
isinstance(sp.parse_expr(1), numbers.Number) == True
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.
So do you mean this block can sometimes return non-complex
value?
if isinstance(result, numbers.Number):
return complex(result)
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 you're sure that complex(result)
is always complex
type, please use ParameterExpression | complex
.
I personally believe that return type annotation should be as concrete as possible, or we need extra type narrowings.
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.
Oops, sorry, I answered without thinking enough: you are right, and after the coercion, the result is complex
indeed.
graphix/parameter.py
Outdated
assert np.allclose(sv.psi, sv2.subs(alpha, 0.5).psi) | ||
""" | ||
|
||
def __init__(self, name): |
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.
Add -> None
.
graphix/parameter.py
Outdated
name : str | ||
name of the parameter, used for binding values. | ||
""" | ||
assert isinstance(name, str) |
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.
Please do not use assert
if you're planning to expose this class.
graphix/parameter.py
Outdated
super().__init__(sp.Symbol(name=name)) | ||
|
||
@property | ||
def name(self): |
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.
Please explicitly specify the return type.
graphix/sim/base_backend.py
Outdated
op_mat = op_mat_from_result(vec, result) | ||
state.evolve_single(op_mat, qubit) | ||
return result | ||
|
||
|
||
class Backend: | ||
def __init__(self, pr_calc: bool = True): | ||
def __init__(self, pr_calc: bool = True, rng: np.random.Generator = None): |
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.
Please use np.random.Generator | None
.
graphix/sim/density_matrix.py
Outdated
|
||
class DensityMatrixBackend(graphix.sim.base_backend.Backend): | ||
"""MBQC simulator with density matrix method.""" | ||
|
||
def __init__(self, pattern, max_qubit_num=12, pr_calc=True): | ||
def __init__(self, pattern, max_qubit_num=12, pr_calc=True, rng: np.random.Generator = None): |
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.
Use | None
.
@shinich1 @thierry-martinez
where we might need to add another line or so to exclude the case where the measurement angles is an instance of |
I think the best way is to move parameter implementation with The reason mostly follows @EarlMilktea 's. Sustaining maintainability is going to help a lot in the long run for everyone contributing to this repo and will help a lot those maintaining.
|
I propose in my last commits a version which does not require
|
Thank you! I believe you mean |
@thierry-martinez the implementation looks good to me! we need to:
|
graphix/parameter.py
Outdated
else: | ||
return NotImplemented | ||
|
||
def __rsub__(self, other) -> Expression | complex | type(NotImplemented): |
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.
Duplicated.
graphix/parameter.py
Outdated
class Expression: | ||
"""Expression with parameters.""" | ||
|
||
def __mul__(self, other) -> Expression | complex | type(NotImplemented): |
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.
Please do not specify NotImplemented
type, as this value itself is never returned from __mul__
.
It's just a marker for internals.
graphix/parameter.py
Outdated
class Expression: | ||
"""Expression with parameters.""" | ||
|
||
def __mul__(self, other) -> Expression | complex | type(NotImplemented): |
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.
Please annotate other
.
graphix/parameter.py
Outdated
def subs(self, variable: Parameter, value: Expression | numbers.Number) -> Expression | complex: | ||
if self == variable: | ||
if isinstance(value, numbers.Number): | ||
return complex(value) |
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.
Type checker complains that Number
cannot be converted to complex
. Could you check it?
graphix/parameter.py
Outdated
def type(self) -> type: | ||
return self.__type | ||
|
||
def subs(self, variable: Parameter, value: Expression | numbers.Number) -> Expression | complex: |
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 once here that numbers.XXX
does not work efficiently with modern typing systems (see Fluent Python 2nd ed.).
Do you come up with alternatives to that?
graphix/parameter.py
Outdated
|
||
|
||
class Compound(Expression): | ||
def __init__(self, children: typing.Iterable[Expression | complex]) -> None: |
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.
typing
is not imported at all.- Please import from
collections.abc
.
graphix/parameter.py
Outdated
return simplify_product(simpl) | ||
|
||
def is_float(self) -> bool: | ||
return lhs.is_float() and rhs.is_float() |
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.
lhs
undefined.
graphix/parameter.py
Outdated
terms[expr] = terms.get(expr, 0) + coef | ||
|
||
|
||
def syntactic_order_key(key: Expression | complex | None) -> Any: |
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.
Please import typing
. Additionally, please do not use Any
if possible as it ruins type annotations.
I'm a little bit concerned about...
|
We suggest the following:
|
This commit implements TeamGraphix#158 (comment) Sympy plugin is implemented in https://github.com/thierry-martinez/graphix-symbolic/tree/sympy
As @shinich1 suggested in the above message, this PR now only provides abstract base classes for parameters, and a minimal concrete class for placeholders (symbolic angles that can solely be substituted, and that cannot appear in computation). The sympy part is implemented as a plugin in the |
This commit implements TeamGraphix#158 (comment) Sympy plugin is implemented in https://github.com/thierry-martinez/graphix-symbolic/tree/sympy
b74c594
to
784dd34
Compare
This commit is yet another tentative to implement parameterized measurement patterns, to fulfill issue TeamGraphix#45 (previous tentative: TeamGraphix#68). This commit adds two methods to the class `Pattern`: - `is_parameterized()` returns True if there is at least one measurement angle that is not just an instance of numbers.Number: indeed, a parameterized pattern is a pattern where at least one measurement angle is an expression that is not a number, typically an instance of `sympy.Expr` (but we don't force to choose sympy here). - `subs(variable, substitute)` returns a copy of the pattern where occurrences of the given variable in measurement angles are substituted by the given value. Substitution is performed by calling the method `subs` on measurement angles, if the method exists, which is the case in particular for `sympy.Expr`. If the substitution returns a number, this number is coerced to `float`, to get numbers that implement the full number protocol (in particular, sympy numbers don't implement `cos`).
`parameter.py` is inspired by TeamGraphix#68 but is split in Parameter and ParameterExpression.
bump above list posted some time ago |
Thank you, @shinich1, for the list!
Fixed in 34c16ed, with a dedicated test.
Should have been fixed with test on random circuits (05c05a0).
Simulations without
I am not sure what to do with QAOA since there is only one pattern in
Placeholders and affine expressions are documented in 149b089.
I added a test to check that there is an exception |
Is it impossible to move symbolic-related ABCs/ |
@EarlMilktea I believe that all the functions ( |
@thierry-martinez |
I added missing |
I re-reviewed you code and felt that the whole logic of |
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.
WIP: May be updated later.
@@ -59,7 +60,9 @@ class RotationInstruction(OneQubitInstruction): | |||
Rotation instruction base class model. | |||
""" | |||
|
|||
angle: float | |||
model_config = ConfigDict(arbitrary_types_allowed=True) |
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.
This is introducing an "escape-hatch" to BaseModel
.
If this is really necessary, let's remove validation itself completely.
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.
Setting arbitrary_types_allowed=True
does not eliminate validation. For types that do not implement their own validation (such as Expression
in this case), the validation defaults to using isinstance
, which is appropriate. Previously, I implemented validation in Expression
specifically using isinstance
(as discussed here: #158 (comment)). Both approaches are equivalent: we can either specify the use of isinstance
for validation within the Expression
implementation, or we can specify it when using Expression
within the model.
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 see. Sorry for my misunderstanding...
By the way, why BaseModel
is introduced here in the first place?
graphix/parameter.py
Outdated
"""Expression with parameters.""" | ||
|
||
@abstractmethod | ||
def __mul__(self, other) -> ExpressionOrFloat: ... |
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.
Please do not omit annotations as I'm planning to enable Ruff rule ANN
(enforce type annotation) for easier code reviews.
(Any
would be acceptable here though)
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.
Fixed in b720e48.
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.
Ah sorry, I made you confused.
My true implication here is that, Any
with @abstractmethod
is acceptable (as we cannot call them at all and the concrete argument types can be overridden), but as usual, not allowed for 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.
Memo: mypy
check results for this file:
graphix/parameter.py:185: error: Argument 1 to "float" has incompatible type "Expression | SupportsFloat"; expected "str | Buffer | SupportsFloat | SupportsIndex" [arg-type]
graphix/parameter.py:282: error: Argument 2 to "subs" of "Expression" has incompatible type "Expression | SupportsFloat"; expected "Expression | float" [arg-type]
graphix/parameter.py:285: error: Incompatible return value type (got "Expression", expected "T | complex") [return-value]
graphix/parameter.py:302: error: Argument 1 to "xreplace" of "Expression" has incompatible type "Mapping[Parameter, Expression | SupportsFloat]"; expected "Mapping[Parameter, Expression | float]" [arg-type]
graphix/parameter.py:305: error: Incompatible return value type (got "Expression", expected "T | complex") [return-value]
op_mat = np.eye(2, dtype=np.complex128) / 2 | ||
else: | ||
# At least one value is symbolic (i.e., parameterized). | ||
op_mat = np.eye(2, dtype="O") / 2 |
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.
dtype="O"
needs extra care as it can impact the performance of simulators.
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 know that symbolic simulations are extra-costly!
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.
Then I suggest completely isolating the logic to different functions: numpy
is notorious for unpredictable automatic type conversion, so I don't want it close to numerical simulator that should be as lightweight as possible.
Current approach (e.g., add conditional branches to existing functions) is very vulnerable to accidental contamination.
Suggested by EarlMilktea: TeamGraphix#158 (comment)
Suggested by EarlMilktea: TeamGraphix#158 (comment)
Suggested by EarlMilktea: TeamGraphix#158 (comment)
Suggested by EarlMilktea: TeamGraphix#158 (comment)
Suggested by EarlMilktea: TeamGraphix#158 (comment)
This partially reverts commit 9f97fda.
This commit is yet another tentative to implement parameterized measurement patterns, to fulfill issue #45 (previous tentative: #68).
This commit adds two methods to the class
Pattern
:is_parameterized()
returns True if there is at least one measurement angle that is not just an instance of numbers.Number: indeed, a parameterized pattern is a pattern where at least one measurement angle is an expression that is not a number, typically an instance ofsympy.Expr
(but we don't force to choose sympy here).subs(variable, substitute)
returns a copy of the pattern where occurrences of the given variable in measurement angles are substituted by the given value. Substitution is performed by calling the methodsubs
on measurement angles, if the method exists, which is the case in particular forsympy.Expr
. If the substitution returns a number, this number is coerced tofloat
, to get numbers that implement the full number protocol (in particular, sympy numbers don't implementcos
).