Conversation
|
Will be covered later (for now just reuse current APIs) |
|
@shinich1 @thierry-martinez @mgarnier59 Sharing my new API design plan. Could you take a look? Changes
|
|
@EarlMilktea thank you - could you remind me why we're not extending
this seems like a safe way indeed. need to be clearly stated throughout the package that we need to cast. |
Because |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #280 +/- ##
==========================================
- Coverage 79.83% 79.56% -0.27%
==========================================
Files 36 36
Lines 5429 4908 -521
==========================================
- Hits 4334 3905 -429
+ Misses 1095 1003 -92 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
MEMOI created Note that there are many drawbacks:
|
|
Hey! We haven't had a chance to have a look yet so I don't really know what this |
This could be covered later with my suggestions. Anyway we need to "hurry up" as this PR is blocking #294 . |
|
If possible, I would suggest splitting implementation changes and API changes into separate PRs. This is generally a good principle, and it becomes especially helpful if we want to move quickly with the implementation changes. More specifically for API changes: | Users need to explicitly cast I think the convention change of introducing a dedicated The precision issue is mostly theoretical: although Python doesn’t formally guarantee IEEE-754 compliance, it’s unclear in what realistic scenarios Moreover, since angle conventions aren’t yet unified between circuits and patterns, any changes related to specialized representations of Pauli angles should also consider the circuit context. | Flow function is now of type Do you mean that only the layer information is returned, and not the full flow? While we mostly use the layer data right now, having access to the full flow could be useful (for example, for circuit extraction). It might also be worthwhile to define dedicated classes for different types of flows. That way, each flow object could carry metadata about its type and provide methods like | Redundant functions are all pruned. That’s always a welcome improvement! Finally, it would be better if the API used |
This is just the thing I wanted to do in the future!
This could be later discussed along with type divergence issues. Anyway, currently I feel
Once I tried it but current codebase is not compatible with |
|
I misunderstood your intention. Please check 2432b3d . |
|
@. all Now |
thierry-martinez
left a comment
thierry-martinez
left a comment
There was a problem hiding this comment.
Nice work. I think we can preserve the existing signatures of find_flow and find_gflow that are currently in master, which only require Plane, as it more precisely expresses what the algorithms require. We could make the signature of find_pauliflow equally precise by requiring Plane | Axis. The conversion from Measurement can be performed in generate_from_graph, as it is done in master. This will remove the need for a dummy angle in _default_construct.
| def get_pauli_nodes( | ||
| meas_planes: dict[int, Plane], meas_angles: dict[int, float] | ||
| ) -> tuple[set[int], set[int], set[int]]: |
There was a problem hiding this comment.
This function does not appear to be used anywhere. It is unclear whether we intend to keep it, and if we do keep it for now, perhaps we should deprecate it.
| def flow_from_pattern(pattern: Pattern) -> tuple[dict[int, set[int]], dict[int, int]]: | ||
| """Check if the pattern has a valid flow. If so, return the flow and layers. |
There was a problem hiding this comment.
Why is this function moved from gflow to gflow_shim? For me, a module gflow_shim would be a good place if we need a compatibility layer between the new gflow module and the former API. But here, flow_from_pattern and gflow_from_pattern implement flow extraction from patterns, which is independent from the flow-finding algorithms implemented in gflow. I don't see why we would want to separate them in another module. More generally, perhaps gflow is not a good module name in the first place and should be renamed just as flow, to group all the flow-related stuff, but this renaming can be considered out of the scope of this PR.
There was a problem hiding this comment.
Because I'm planning to someday remove this.
As Pattern -> graph conversion is already implemented, there is no good reason for maintaining redundant direct transformations.
I suppose, this kind of redundant transformation APIs are one of the chief reasons for current densely-connected module/class structures.
There was a problem hiding this comment.
These functions do not perform the same computation.
- The functions
find_flow,find_gflow, andfind_pauliflowsearch for a flow in a given graph, but there may be multiple valid flows in the same graph with different correction strategies. - The functions
flow_from_pattern,gflow_from_pattern, andpauliflow_from_pattern, return the unique flow (if it exists) that corresponds to the correction strategy implemented in a given pattern.
Thus, there is no redundancy between these functions. Here are some consequences of this difference:
- The round-trip
OpenGraph.to_pattern∘find_pauliflow∘OpenGraph.from_patterndoes not necessarily yield the original pattern. - By contrast, starting from a pattern
$p$ , if$g$ is the open graph of$p$ and$f$ is the flow of$p$ , we can reconstruct$p$ with$g$ and$f$ modulo commutation rules. Currently, we cannot provide a fixed flow togenerate_from_graph, but there is an open issue addressing this (Add optional argument to pass gflow ingenerate_from_graph()#120). - The function
visualize_from_patternshows the actual correction strategy of a given pattern, extracted withflow_from_pattern. By contrast, if we just usevisualize, the correction strategy that is drawn may differ from the one implemented by the pattern. - If these functions are removed, we will have no way to check if a given pattern has a flow: using
find_flowmay return a valid flow from the pattern's open graph, but there is currently no way to check if the pattern itself implements the correction strategy of that flow (it would be interesting to have a function that checks whether a given pattern implements a given flow, Add a function that checks if a pattern implements a given flow #306).
There was a problem hiding this comment.
For instance, the bug #157 occurs because find_flow is used in place of flow_from_pattern. I have included an example at: #157 (comment).
There was a problem hiding this comment.
Hi @thierry-martinez (CC: @mgarnier59 @shinich1 @EarlMilktea )
Let me have two comments.
-
gflow_from_pattern does not always find an existing gflow
As far as I remember correctly, this function is just a trial patch to find the flow consistent with the Pattern corrections in thevisualizermodule. There’s a case wheregflowcan find one, butgflow_from_patternfails. In this case, a pattern is actually generated from some gflow, but the appearance is modified by Pauli simplification described in the Measurement Calculus paper. I mean, if the Graphix team plans to use this in many places, I hope you remember this context. -
Multiple solutions of gflow
The bug reported inminimize_spacebreaks runnability for patterns that are inconsistent with their graph flow #157 seems to come from the ambiguity of input_nodes rather than the flow itself. If there are multiple solutions for flow (including gflow and Pauli flow) under a sufficient number of input and output, they are all equivalent to each other modulo stabilizer operators. Therefore, it’s in principle not the problem to use onlygflowmethod. I agree there are some problems when you do something from Pattern to Pattern, though.
There was a problem hiding this comment.
Hi @masa10-f. In #157, only causal flows are relevant, as if there is only a gflow but no causal flow, we do not know optimal bounds on space. There is no ambiguity regarding input node (which is fixed). I agree that all flows are equivalent with respect to the unitary that is computed, but the point is that they differ in the measurement order they impose by their correction strategies.
Your first point suggests some bugs in gflow_from_pattern: it would be interesting if you could fill an issue with an example. There is also a bug in pauliflow_from_pattern, since this function calls find_pauliflow rather than considering whether the pattern uses a different correction strategy. Anyway, if these functions have bugs, that means that the bugs should be fixed, not that the functions should be removed!
There was a problem hiding this comment.
I understand this function cannot be removed now.
someday remove this
Do you think it is ever possible in the future? I still believe that we should not define conversions for all the pairs (or module structure becomes too dense)
There was a problem hiding this comment.
@thierry-martinez Thank you for your reply.
- Ambiguity of input_nodes
Sorry for the confusion. What I meant is that when the number of input nodes is smaller than the number of output nodes, there is flow ambiguity corresponding to the missing input nodes.
For example, in the case of #157, there are two possible input states, resulting in two valid solutions. The starting points of the two flow paths differ:
- One is: 3 → 1 → 4 → 5 and 0 → 2
- The other is: 3 → 0 → 2 and 1 → 4 → 5
Even if the full set of input qubits is not specified in the pattern, the flow always assumes a sufficient number of input nodes. Therefore, these represent fundamentally different types of patterns.
I meant that the difference lies not only in the measurement order, but also in the input states themselves, which are not necessarily in the |+⟩ state.
I agree with you that minimize_space has a critical bug. This is also an immature patch at the early stage of development.
There was a problem hiding this comment.
- flow from graph
I assumed the following situation, but I found find_pauliflow inside the pauliflow_from_pattern successfully avoided my concern.
# %%
from graphix.fundamentals import Plane
from graphix.pattern import Pattern
from graphix.command import N, E, M, X, Z
from graphix.gflow import (
flow_from_pattern,
pauliflow_from_pattern,
)
# %%
# pattern construction
# simple chain graph: 0 (in) - 1 - 2 (out)
pattern = Pattern(
input_nodes=[0],
cmds=[
N(1),
N(2),
E((0, 1)),
E((1, 2)),
M(0, Plane.XY, 1 / 3, set(), set()),
M(1, Plane.XY, 0, {0}, set()), # Pauli X measurement
X(2, {1}),
Z(2, {0}),
],
output_nodes=[2],
)
# apparently, this pattern has a trivial flow {0: 1, 1: 2}
flow, l_k = flow_from_pattern(pattern)
print("Flow:", flow)
# %%
# This pattern can be simplified with Pauli simplification procedure.
# The equations are described in the top of 15p [Measurement Calculus paper](https://arxiv.org/pdf/0704.1263)
pattern_simp = Pattern(
input_nodes=[0],
cmds=[
N(1),
N(2),
E((0, 1)),
E((1, 2)),
M(0, Plane.XY, 1 / 3, set(), set()),
M(
1, Plane.XY, 0, set(), set()
), # Pauli X measurement. The s_domain can be eliminated
X(2, {1}),
Z(2, {0}),
],
output_nodes=[2],
)
# The correction flow acquired is:
# flowlike = {1, 2}. Here, the oddneighbors are Odd(flowlike[0]) = {}, Odd(flowlike[1]) = {} (I eliminated the self-loop for simplicity)
# They are not consistent with the original pattern_simp strategy: Z{0: 2}
# Moreover, the correction strategy in pattern_simp is not consistent with Pauli flow conditions.
# The below will fail if the method directly extract corrections from pattern. However, it succeeds now because it calls `find_pauliflow` so that it properly finds ignored_nodes.
pauli_flow, l_k = pauliflow_from_pattern(pattern_simp)
print("Pauli flow:", pauli_flow)
# This suggests that the correction strategy on the pattern should be interpreted considering the ignored nodes. If you generate a pattern from the direct correction strategy, it will not work.
# %%| def group_layers(l_k: Mapping[int, int]) -> tuple[int, dict[int, set[int]]]: | ||
| """Group nodes by their layers. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| graph: :class:`networkx.Graph` | ||
| Graph (incl. input and output) | ||
| iset: set | ||
| set of node labels for input | ||
| oset: set | ||
| set of node labels for output | ||
| meas_planes: dict | ||
| measurement planes for each qubits. meas_planes[i] is the measurement plane for qubit i. | ||
| k: int | ||
| current layer number. | ||
| l_k: dict | ||
| layers obtained by gflow algorithm. l_k[d] is a node set of depth d. | ||
| g: dict | ||
| gflow function. g[i] is the set of qubits to be corrected for the measurement of qubit i. | ||
| mode: str(optional) | ||
| The gflow finding algorithm can yield multiple equivalent solutions. So there are three options | ||
| - "single": Returrns a single solution | ||
| - "all": Returns all possible solutions | ||
| - "abstract": Returns an abstract solution. Uncertainty is represented with sympy.Symbol objects, | ||
| requiring user substitution to get a concrete answer. | ||
| layers obtained by flow or gflow algorithms | ||
|
|
||
| Returns | ||
| ------- | ||
| g: dict | ||
| gflow function. g[i] is the set of qubits to be corrected for the measurement of qubit i. | ||
| l_k: dict | ||
| layers obtained by gflow algorithm. l_k[d] is a node set of depth d. | ||
| d: int | ||
| minimum depth of graph | ||
| layers: dict of set | ||
| components of each layer | ||
| """ |
There was a problem hiding this comment.
Better documentation of the input and output mappings would be helpful to understand what this function computes.
"""Group nodes by their layers.
This function converts a mapping from each node to its layer (as
obtained from flow or gflow algorithms) into a mapping from each
layer to the set of nodes it contains.
Parameters
----------
l_k: dict
Mapping from nodes to layers, as produced by flow or gflow algorithms.
Returns
-------
d: int
Minimum depth of graph.
layers: dict of set
Mapping from each layer to the set of nodes it contains.
"""| def _default_construct(keys: Iterable[int]) -> dict[int, Measurement]: | ||
| # Random angle for safety | ||
| return dict.fromkeys( | ||
| keys, | ||
| Measurement(Plane.XY, 0.5014943209046647), | ||
| ) |
There was a problem hiding this comment.
Having a dummy value for measurement angles looks a bit awkward and is a sign that Measurement is not the right data structure to use in this module. I hope that we can eliminate it with the refactoring I propose in this review: use Measurement in OpenGraph and generate_from_graph, but use Plane and PPlane in gflow.
| graph: nx.Graph[int], | ||
| iset: AbstractSet[int], | ||
| oset: AbstractSet[int], | ||
| meas: Mapping[int, Measurement] | None = None, |
There was a problem hiding this comment.
The former type for meas_planes (dict[int, Plane]) expresses precisely what is required to compute a flow. I think we should keep it, in its abstract form Mapping[int, Plane], rather than requiring unnecessary information. The function generate_from_graph will be responsible for converting Measurement to Plane before calling find_flow. The name meas_planes seemed appropriate but if we want something shorter, we could use just planes (although meas on its own would be misleading).
| Input nodes. | ||
| oset : `collections.abc.Set` | ||
| Output nodes. | ||
|
|
There was a problem hiding this comment.
The documentation of the parameter meas is missing. Is that what you meant on line 154?
| if meas is None: | ||
| meas = _default_construct(graph.nodes - oset) |
There was a problem hiding this comment.
The former implementation, where meas_planes defaulted to Plane.XY, worked fine, and was more principled.
| graph: nx.Graph[int], | ||
| iset: AbstractSet[int], | ||
| oset: AbstractSet[int], | ||
| meas: Mapping[int, Measurement] | None = None, |
There was a problem hiding this comment.
As for find_flow, I would prefer to retain the earlier version Mapping[int, Plane], and perform the conversion in generate_from_graph.
| graph: nx.Graph[int], | ||
| iset: AbstractSet[int], | ||
| oset: AbstractSet[int], | ||
| meas: Mapping[int, AnyMeasurement] | None = None, |
There was a problem hiding this comment.
The type we want here is something isomorphic to Mapping[int, Plane | Axis], and to perform the conversion from Measurement in generate_from_graph.
|
I'll close this PR and create a new one for easier reviews. |
5 participants
Closes #276 .
MEMO
git+in requirements.