-
Notifications
You must be signed in to change notification settings - Fork 41
Refactor of flow tools - Flow from pattern #393
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
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #393 +/- ##
==========================================
- Coverage 86.10% 84.91% -1.19%
==========================================
Files 45 46 +1
Lines 6576 6637 +61
==========================================
- Hits 5662 5636 -26
- Misses 914 1001 +87 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
CI is failing on the visualization example with Pauli presimulation. Should pass when #392 is merged. |
| meas_order = None | ||
| if not self._pauli_preprocessed: | ||
| meas_order = self.get_measurement_order_from_flow() | ||
| cf = self.extract_opengraph().find_causal_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.
Note that this change preserves bug #157, but I think fixing it would be outside the scope of this PR. Ideally, we would like cf = self.try_extract_causal_flow(), but this may cause some regressions: minimize_space will be correct in the sense that it will preserve runnability, but in some examples the minimized pattern may use more space than the current heuristics.
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.
Agree, thanks for the remark.
Co-authored-by: thierry-martinez <thierry.martinez@inria.fr>
|
Thanks a lot for the detailed review, @thierry-martinez. I applied all suggestions in 596ec4b and 975f7aa. I think it's ready for another round! |
This commit updates the baseline while TeamGraphix#392 is not merged.
thierry-martinez
left a comment
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.
LGTM!
mgarnier59
left a comment
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.
Looks good, nothing to add. Thanks!
This PR introduces some tooling to extract a causal flow and a generalised flow from a pattern without running the extraction algorithms on the underlying open graph.
It directly addresses #306 and additionally solves #367.
Description of changes
We introduce the following methods:
graphix.optimization.StandardizedPattern.extract_partial_order_layerspartial_order_layersattribute of:class: PauliFlowand:class: XZCorrectionsobjects :imust be measured after nodes in layeri + 1.graphix.optimization.StandardizedPattern.extract_causal_flowgraphix.optimization.StandardizedPattern.extract_partial_order_layers.CausalFlowis constructed combining these you objects.CausalFlowobject is checked.graphix.optimization.StandardizedPattern.extract_gflowgraphix.optimization.StandardizedPattern.extract_opengraphgraphix.pattern.Pattern.extract_partial_order_layers,graphix.pattern.Pattern.extract_causal_flowandgraphix.pattern.Pattern.extract_gflowWe introduce the a new module
graphix.flow._partial_order.pywith the function:func: compute_topological_generations.This function was used in the algebraic Pauli-flow finding routine and is now used to extract the partial order from the pattern.
In the future, we should drop the dependency of
graphix.flow.coreonnx.DiGraphobjects and replace:func: graphix.flow.core._dag_to_partial_order_layerswith this function.Dependencies of the module
graphix.visualization.pyongraphix.gflow.pyare removed where possible, but the new flow API is not fully exploited. This is out of the scope of this PR.