Open thierry-martinez opened 1 month ago
Attention: Patch coverage is 81.87050%
with 126 lines
in your changes missing coverage. Please review.
Project coverage is 72.95%. Comparing base (
ec4c582
) to head (5feac05
). Report is 5 commits behind head on master.
Files | Patch % | Lines |
---|---|---|
graphix/parameter.py | 80.54% | 122 Missing :warning: |
graphix/transpiler.py | 80.95% | 4 Missing :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
@thierry-martinez great, is the intention to merge @pafloxy's #68-equivalent code first (when it's ready), before merging this?
@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 Parameter
class in the commit 81bbeb7 I just pushed. #68 was incorrect because arithmetic operators changed parameters in place (computing alpha + 1
did change the value of alpha
to alpha + 1
!). In this PR, we distinguish between Parameter
(alpha
) and ParameterExpression
(alpha + 1
), and a Parameter
is a particular case of ParameterExpression
.
The test test_parameter_simulation
is illustrative: we can either substitute a parameter by a value in a pattern then simulate, or simulate symbolically (given that the pattern is deterministic!) then substitute in the (symbolic) state vector or the density matrix, and we get the same result.
@thierry-martinez @pafloxy great! give us a few days to look through. A few quick comments:
requirements.txt
?circuit.simulate_statevector
, pattern.perform_pauli_measurements
, as well as visualization tool, work with parameters?Let me review it later.
Anyway, is it absolutely necessary to use sympy? I'm concerned with...
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.
@thierry-martinez @pafloxy great! give us a few days to look through. A few quick comments:
- is TN backend working with parameters, or should we throw error in TN backend if pattern is parametrised? ~2. could you add sympy to
requirements.txt
?~- will
circuit.simulate_statevector
,pattern.perform_pauli_measurements
, as well as visualization tool, work with parameters?
@shinich1 @thierry-martinez
Maybe you guys figured it already but I think we need to make some small modification to the visualization.py
module as well. There are some checks to determine if a measurement angle is Pauli in some of the class methods for GraphVisualizer
of the form
elif (
show_pauli_measurement
and self.meas_angles is not None
and (
2 * self.meas_angles[node] == int(2 * self.meas_angles[node])
) # measurement angle is integer or half-integer
):
where we might need to add another line or so to exclude the case where the measurement angles is an instance of Parameter
/ParameterExpression
.
Hi @pafloxy @thierry-martinez
I think the best way is to move parameter implementation with sympy
out of main graphix
repository, for example a separate repo (wrapper/module) dedicated to parameterized patterns and their executions (it seems possible, given the relatively small changes required to implement?). I am happy to initiate a creation of such repository in teamgraphix
organization, if that makes sense to you? for example, that can be part of extra
pip installation.
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.
sympy
has quite a performance issue. Looking at their github repo, it seems like a recurring issue in qiskit
(which has sympy
incorporated) and quite a bit of effort can be saved by deciding to move it out at this point (nb as @EarlMilktea mentioned sympy
will be removed from requirements.txt
when gflow
is refactored for performance soon.)I propose in my last commits a version which does not require mypy
.
The module parameter.py
implements symbolic expressions so as to support parameters, but without all the symbolic. machinery of mypy
(computations with known values are done numerically, not symbolically).
The rest of the code does not depend on parameter.py
: in particular, the code for simulators is generic. If we still need the sympy
version of parameters, we can implement it separately, and simulators should work without any specific modification in the code of graphix.
Tests for parameters are done in a specific test_parameter.py
module, and the tests include circuit simulation.
I propose in my last commits a version which does not require
mypy
.
- The module
parameter.py
implements symbolic expressions so as to support parameters, but without all the symbolic. machinery ofmypy
(computations with known values are done numerically, not symbolically).- The rest of the code does not depend on
parameter.py
: in particular, the code for simulators is generic. If we still need thesympy
version of parameters, we can implement it separately, and simulators should work without any specific modification in the code of graphix.- Tests for parameters are done in a specific
test_parameter.py
module, and the tests include circuit simulation.
Thank you! I believe you mean sympy
instead of mypy
above? Let me take a detailed look at the code soon.
@thierry-martinez the implementation looks good to me! we need to:
rz
only with no arithmetic of param covered?)subs
? if so we should throw error if parameterized and being simulated?examples
.docs
(just autodoc, not much to write - see existing pages)subs
before PatternRunner
initialization.I'm a little bit concerned about...
parameter.py
? It's already really huge.We suggest the following:
graphix
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
).