-
Notifications
You must be signed in to change notification settings - Fork 2
Implement layer inference #18
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
base: master
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #18 +/- ##
==========================================
+ Coverage 95.35% 95.99% +0.64%
==========================================
Files 16 16
Lines 925 1024 +99
Branches 27 48 +21
==========================================
+ Hits 882 983 +101
+ Misses 43 40 -3
- Partials 0 1 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
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 would not expect to require the graph to be connected: we would like the flow algorithms to work even for graphs with multiple connected components.
- What
infer_layer
computes is unclear, and it rejects some flows that appear to be valid (such as some flows returned bypflow.find
). - The implemented notion of
ensure_optimal
is less strict than the maximally-delayed condition.
python/swiflow/common.py
Outdated
|
||
def infer_layer(g: nx.Graph[_V], anyflow: Mapping[_V, _V | AbstractSet[_V]]) -> Mapping[_V, int]: |
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 would expect the name to be
infer_layers
, since there are potentially multiple layers. - Is
infer_layer
supposed to find the layers for any flow returned byfind_pflow
? I got an error with the following test, that finds a pflow and then try to find the layers:
from __future__ import annotations
from math import pi
from typing import assert_never
from graphix import Circuit
from graphix.instruction import CNOT, RY
from graphix.measurements import Measurement, PauliMeasurement
from graphix.fundamentals import Axis, Plane
from graphix.opengraph import OpenGraph
from swiflow import common
from swiflow.common import PPlane
import swiflow.pflow
def measurement_to_pplane(measurement: Measurement) -> PPlane:
if pm := PauliMeasurement.try_from(measurement.plane, measurement.angle):
if pm.axis == Axis.X:
return PPlane.X
elif pm.axis == Axis.Y:
return PPlane.Y
elif pm.axis == Axis.Z:
return PPlane.Z
if measurement.plane == Plane.XY:
return PPlane.XY
if measurement.plane == Plane.XZ:
return PPlane.XZ
if measurement.plane == Plane.YZ:
return PPlane.YZ
assert_never(measurement.plane)
def test_infer_layer() -> None:
circuit = Circuit(
width=2,
instr=[
CNOT(0, 1),
RY(0, pi / 4),
],
)
pattern = circuit.transpile().pattern
og = OpenGraph.from_pattern(pattern)
meas_pplanes = {
node: measurement_to_pplane(measurement)
for node, measurement in og.measurements.items()
}
pflow, _layers = swiflow.pflow.find(
og.inside, set(og.inputs), set(og.outputs), pplane=meas_pplanes
)
assert pflow is not None
_ilayers = common.infer_layer(
og.inside, pflow
) # raises ValueError: Failed to determine layer for all nodes.
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.
infer_layers
Do you come up with another name that better incorporates its nature of returning only one candidate?
I admit current name is not the best.
any flow returned by
find_pflow
Currently pflow is not supported (this is why pflow unit test is missing!)
I suppose we need to drop some order constraints from the input flow, specifically from where Pauli-specific MC simplifications are exploited: I have an idea but not implemented for now.
Could you help me with debugging if I suggest in this PR?
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.
Added ee1ae77 .
In _special_edges
, all the exceptional edges (u, v)
where v
is either
X
-measured + inf[u]
:X
in the stabilizer is absorbed intoM^X
Z
-measured + inOdd(f[u])
:Z
in the stabilizer is absorbed intoM^Z
Y
-measured + in bothf[u]
andOdd(f[u])
: overall contributions tov
reduces toY
absorbed intoM^Y
are enumerated and later these edges are pruned from partial order constraints.
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.
Do you come up with another name that better incorporates its nature of returning only one candidate?
infer_layers
is fine, since we expect there will be only one returned value, but if you want a singular noun, you can choose infer_layering
.
Added ee1ae77 .
There is still an issue with [CNOT(0, 1), CNOT(0, 1), RY(0, pi / 4)]
.
Could you help me with debugging if I suggest in this PR?
Of course, but I first would like to know why we need such a function infer_layer*
and why we want to implement it in swiflow
if it's pure Python.
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 want to implement it in
swiflow
if it's pure Python
Main reason is to align verification API to the graphix's in a locally-closed way.
(MEMO: I'm now planning to automatically call infer_layer
inside verify_xxx
if missing, and make infer_layer
private as it's usage is a bit tricky.
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:
infer_layer
is necessary because xxxflow_from_pattern
in graphix
proceeds as follows:
- Identify
M
->C
dependencies - Translate it to a corresponding flow-like
- Return it if valid
So we somehow complement missing layer
information.
infer_layer
is the generalization of related functionalities implemented in current graphix
.
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 also updated the name from infer_layer
to infer_layers
in aeb0e8a .
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.
Indeed, since the flow_from_pattern
functions in Graphix return a pair consisting of a flow and the corresponding minimal layering, they need to compute this layering. Note, however, that the layering is not part of the formal definition of a flow. It just so happens that flow-finding algorithms often compute it as a side product, so it is natural to return it as well, since it has already been computed.
However, one could imagine not computing the layering if it is not obtained for free as a side product, and instead letting the user compute it on demand by explicitly calling infer_layers
. In particular, I would expect find_flow
to return the layering in addition to the flow, but not flow_from_pattern
, since the latter does not necessarily need to.
In any case, we were previously discussing the verify_flow
functions, and I still don't see the role of the layering in these functions. More precisely, there are two kinds of flow verification:
-
Given an open graph and a flow, verify that the flow is well-formed;
-
Given a pattern and a flow, verify that the pattern implements the given flow.
In both cases, the layering does not play any role.
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.
layering does not play any role
Reasonable. I will update verification APIs based on this idea.
Yet, I believe we should keep full-verification features (verify both geometric and order requirements) in swiflow
as they're the most straightforward implementation of the original papers.
Of course, their visibility (whether users can access to them or not) is a different problem.
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.
Now layer-related checks are skipped if missing.
python/swiflow/pflow.py
Outdated
ensure_optimal : `bool` | ||
Whether the pflow should be maximally-delayed. Defaults to `False`. |
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.
What is the exact condition you would like to check for ensure_optimal
?
Currently, validate::check_initial
only checks that the layer 0 is the layer of the output nodes. The following example delays non-output nodes in successive useless layers without being caught by ensure_optimal
:
from typing import assert_never
from graphix import Circuit
from graphix.measurements import Measurement, PauliMeasurement
from graphix.opengraph import OpenGraph
from graphix.fundamentals import Axis, Plane
from swiflow.common import PPlane
import swiflow.pflow
def measurement_to_pplane(measurement: Measurement) -> PPlane:
if pm := PauliMeasurement.try_from(measurement.plane, measurement.angle):
if pm.axis == Axis.X:
return PPlane.X
elif pm.axis == Axis.Y:
return PPlane.Y
elif pm.axis == Axis.Z:
return PPlane.Z
if measurement.plane == Plane.XY:
return PPlane.XY
if measurement.plane == Plane.XZ:
return PPlane.XZ
if measurement.plane == Plane.YZ:
return PPlane.YZ
assert_never(measurement.plane)
def test_optimal():
circuit = Circuit(2)
circuit.h(0)
circuit.cnot(0, 1)
pattern = circuit.transpile().pattern
og = OpenGraph.from_pattern(pattern)
inputs = set(og.inputs)
outputs = set(og.outputs)
meas_pplanes = {
node: measurement_to_pplane(measurement)
for node, measurement in og.measurements.items()
}
pflow, layers = swiflow.pflow.find(og.inside, inputs, outputs, pplane=meas_pplanes)
layers = {node: node + 1 if layer > 0 else 0 for node, layer in layers.items()}
swiflow.pflow.verify(
(pflow, layers),
og.inside,
inputs,
outputs,
pplane=meas_pplanes,
ensure_optimal=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.
I may need to first "standardize" layers to...
- use
0
as the minimum layer - use contiguous integers
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 completely forgot about minimum layer verification already added in this PR.
As min(layers) == 0
is guaranteed and validate::check_initial
is the only place where the value of layer[u]
itself matters (only order statistics are tested in other occurrences), the above behavior you reported is not a bug, i.e., my code correctly recognizes 1
is not used and thus the input flow has minimum number of distinct layers.
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.
In the example I provided, layers
is {0: 1, 1: 2, 2: 0, 3: 4, 4: 0}
, that is, there are 4 different layers: 0, 1, 2, and 4, rather than just 2 layers (the original value for layers
is {0: 1, 1: 1, 2: 0, 3: 1, 4: 0}
).
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.
Let's not call it optimal
because optimality depends on context (Mhalla&Perdrix paper say optimal
flow but this only means the depth is minimal; there are many ways for things to be optimal depending on your requirement). how about being more explicit like ensure_maximally_delayed
)
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.
The function infer_layers
does not compute any flow (maximally-delayed or otherwise); instead, it takes a flow as input and returns a layering corresponding to that flow. The layering returned by infer_layers
is minimal in the sense that there is no other layering compatible with the given flow that assigns a strictly smaller layer to any node.
However, this is not directly related to the notion of a maximally-delayed flow, since the function is defined for any flow, whether maximally-delayed or not. It just so happens that the layering associated with a maximally-delayed flow is pointwise less than the layering for any other flow, but infer_layers
simply computes the layering for the specific flow it is given and does not attempt to find the maximally-delayed one.
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.
the function is defined for any flow
Oh I forgot about this.
Then an appropriate wording here might be "infer_layers
minimizes the number of distinct layers" and the word "maximally-delayed" should not be included at all.
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.
The function infer_layers
returns the layering that is pointwise minimal among all the layerings compatible with the given flow, in the sense given above: for every node
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.
(In particular, it minimizes the number of distinct layers; however the characterization in terms of pointwise minimality is stronger.)
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.
Now ensure_optimal
is removed.
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.
Pull Request Overview
This PR implements layer inference functionality that allows computing flow layers from input flow mappings, replacing the need for xflow_from_pattern
features in graphix
. The implementation includes layer inference for all flow types (flow, gflow, pflow) with validation and suboptimal flow verification capabilities.
Key changes:
- Adds
infer_layer
function to automatically compute layers from flow mappings - Modifies verification functions to support both optimal and suboptimal flows
- Updates test cases to verify both optimal and suboptimal scenarios
Reviewed Changes
Copilot reviewed 23 out of 24 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
python/swiflow/_common.py |
Implements core infer_layer function and helper utilities for layer computation |
python/swiflow/flow.py |
Updates flow verification to support layer inference and optional optimality checks |
python/swiflow/gflow.py |
Updates gflow verification to support layer inference and optional optimality checks |
python/swiflow/pflow.py |
Updates pflow verification to support layer inference and optional optimality checks |
python/swiflow/common.py |
Simplifies type definitions by removing dataclass wrappers and using type aliases |
src/flow.rs |
Adds optimal parameter to verification function |
src/gflow.rs |
Adds optimal parameter to verification function |
src/pflow.rs |
Adds optimal parameter to verification function |
tests/test_*.py |
Updates tests to use new API and adds layer inference verification tests |
tests/assets.py |
Updates test data format and adds new test cases |
Comments suppressed due to low confidence (2)
python/swiflow/_common.py:53
- The parameter name
plike
is ambiguous. Consider using a more descriptive name likeplane_mapping
ormeasurement_planes
to clarify its purpose.
def check_planelike(vset: AbstractSet[_V], oset: AbstractSet[_V], plike: Mapping[_V, _P]) -> None:
tests/test_common.py:157
- The test expects a specific error message pattern with
match=r".*determine.*"
but the actual error message from the function is "Failed to determine layer for all nodes." Ensure the test regex pattern accurately matches the expected error message.
_common.infer_layer(g, flow)
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 propose a more efficient implementation for infer_layers
.
def _codec_wrap( | ||
codec: IndexMap[_V], | ||
pflow: tuple[_PFlow[_V], _Layer[_V]] | _PFlow[_V], | ||
) -> tuple[dict[int, set[int]], list[int] | None]: | ||
if isinstance(pflow, tuple): | ||
f, layers = pflow | ||
return codec.encode_gflow(f), codec.encode_layers(layers) | ||
return codec.encode_gflow(pflow), 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.
This function is identical to gflow._codec_wrap
. Can we introduce a shared implementation in _common
?
if let Some(layers) = layers { | ||
check_def_layer(&f, &layers, &g)?; | ||
} |
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 no layering is given, we should still check that there is a partial order compatible with the flow. You can either use infer_layers
for that, or just check that the dependency graph forms a DAG. The same remark applies for the other verify
functions.
import networkx as nx
import swiflow.flow
def test_verify() -> None:
g = nx.Graph([(0, 2), (1, 2)])
iset = {0, 1}
oset = {2}
# Expected: a pattern where |I| > |O| cannot admit a flow
assert swiflow.flow.find(g, iset, oset) == None
flow = {0: 2, 1: 2}
# Unexpected success: this should be rejected because:
# - f(0) = 2 implies that 0 ≺ 1, since 1 ∈ N(2)
# - f(1) = 2 implies that 1 ≺ 0, since 0 ∈ N(2)
# but we cannot have both 0 ≺ 1 and 1 ≺ 0.
swiflow.flow.verify(flow, g, iset, oset)
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.
Isn't it contradicting this thread #18 (comment) ?
I would like to know whether you later noticed it is actually necessary (layer does play an important role), or I'm still missing some bits.
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.
Okay, sorry, I should soften my statement:
-
In the context of
flow_from_pattern
(the thread of the comment you cited), the corrections don’t play any role when verifying that a pattern implements a given flow. -
In the context of checking a flow with respect to an open graph (the current thread, which I mistakenly invoked in the comment you cited), the order of the corrections is only relevant because we need to check that it forms a DAG. Using
infer_layers
is one way to verify that, although it computes more than we actually need here.
/// - XY: i not in g(i) and in Odd(g(i)) | ||
/// - YZ: i in g(i) and in Odd(g(i)) | ||
/// - XZ: i in g(i) and not in Odd(g(i)) | ||
fn check_def_geom(f: &GFlow, g: &[Nodes], planes: &Planes) -> Result<(), FlowValidationError> { | ||
for &i in itertools::chain(f.keys(), planes.keys()) { | ||
if f.contains_key(&i) != planes.contains_key(&i) { | ||
Err(InvalidMeasurementSpec { node: i })?; | ||
} | ||
} | ||
for (&i, fi) in f { | ||
let pi = planes[&i]; |
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.
In the definition of a gflow, we should have
import math
import networkx as nx
import numpy as np
from graphix.fundamentals import Plane
from graphix.states import PlanarState
from graphix.generator import _gflow2pattern
import swiflow.gflow
import swiflow.common
def test_gflow() -> None:
graph = nx.Graph([(0, 1)])
inputs = {0}
outputs = {1}
planes_swiflow = {0: swiflow.common.Plane.XZ}
# Expecting `None`: this graph does not admit a flow
g, l_k = swiflow.gflow.find(graph, inputs, outputs, planes=planes_swiflow)
planes = {0: Plane.XZ}
angles = {0: 0.1}
pattern = _gflow2pattern(graph, angles, inputs, planes, g, l_k)
input_state = PlanarState(Plane.XY, 0.7)
# Checks that the pattern is deterministic (as a pattern with a gflow should be)
state_ref = pattern.simulate_pattern(input_state=input_state)
for _ in range(25):
state = pattern.simulate_pattern(input_state=input_state)
assert math.isclose(np.abs(np.dot(state.flatten().conjugate(), state_ref.flatten())), 1)
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 it locally: will push later.
By the way, I suspect pflow finding algorithm introduced in https://arxiv.org/abs/2109.05654 could violate
For example, in the first iteration of PauliFlowAux(V,Γ,I,λ,A,B,k)
with B = O
, forall u
visits u in I
and thus u
can be included in Kxz
and Kxy
, possibly leading to
I welcome your comments on this observation.
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.
Yes, this side condition is not checked by the published algorithm and should be checked in addition.
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.
Is it still necessary for "special edges" (see my python impl. in this PR for the terminology)?
I suppose we could safely apply C
after some specific Pauli M
inside I
, as we do for V \ I
.
This PR implements
infer_layer
, which infers flow layer from input flow mapping.This function is expected to replace
xflow_from_pattern
features ingraphix
.Minor Changes