Add Open graph class

[wlcsm]

Context: Many researchers in MBQC use a graphical approach to reasoning about patterns. These center around the concept of a open graph which can be thought of as a graphical description of an MBQC pattern with inputs and outputs

Description of the change: Added the Open graph class together with functionality for converting to and from pyzx diagrams.

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 86.80556% with 19 lines in your changes missing coverage. Please review.

Project coverage is 75.89%. Comparing base (364be19) to head (71472ca). Report is 1 commits behind head on master.

Files	Patch %	Lines
graphix/opengraph.py	82.25%	11 Missing :warning:
graphix/pyzx.py	90.24%	8 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #191      +/-   ##
==========================================
+ Coverage   75.52%   75.89%   +0.37%     
==========================================
  Files          34       36       +2     
  Lines        5569     5713     +144     
==========================================
+ Hits         4206     4336     +130     
- Misses       1363     1377      +14     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[wlcsm]

Hi @EarlMilktea , thanks for your review! I've replied to a couple of your suggestions to get your opinion first and I've implemented the rest of the suggestions and pushed it to this PR.

Could you have a look at my responses please?

[shinich1]

thanks for the PR @wlcsm ! Just to understand the direction:

• is the expected workflow ZX -> opengraph-> generate_from_graph -> pattern?
• for the opposite workflow (pattern->opengraph->PYZX) do(n't) we want to check the compatibility with gflow so that we can go pat-ZX-back to pat while maintaining semantics?

[wlcsm]

My pleasure :)

is the expected workflow ZX -> opengraph-> generate_from_graph -> pattern?

Yes

for the opposite workflow (pattern->opengraph->PYZX) do(n't) we want to check the compatibility with gflow so that we can go pat-ZX-back to pat while maintaining semantics?

I don't think this necessary in the open graph -> ZX or ZX -> open graph code. A ZX graph does not require flow. If you want to convert it to a ZX circuit (not a graph) then you will likely use the zx.extract_circuit function which will check for flow when creating the graph.

[shinich1]

for the opposite workflow (pattern->opengraph->PYZX) do(n't) we want to check the compatibility with gflow so that we can go pat-ZX-back to pat while maintaining semantics?

I don't think this necessary in the open graph -> ZX or ZX -> open graph code. A ZX graph does not require flow. If you want to convert it to a ZX circuit (not a graph) then you will likely use the zx.extract_circuit function which will check for flow when creating the graph.

yes if it's graph <> ZX, there's no flow involved. Once we involve patterns, we need to care about flows, and what type (flow/gflow/pailiflow, max-delayed, focused, other). This is what @masa10-f has worked on, which is perhaps something we can incorporate later as a feature-rich transpilation between open graph and pattern (to be in separate PR).

separate from above, do we want to implement OpenGraph.to_pattern to wrap generate_from_graph, as well as write up bare minimum implementation of OpenGraph.from_pattern (i.e. just extarct the open graph, noting limitation of it)?

[shinich1]

also - @EarlMilktea @thierry-martinez do you think we should keep these qasm files? I get that they're useful for generating a bunch of zx diagrams and that pyzx has similar things (might also be good for grapihix' own testing once we have qasm input).

[EarlMilktea]

@shinich1

I personally don't prefer to include .qasm files or even pyzx due to package quality issues. They should be replaced with self-hosted implementations, at least in the future.

[wlcsm]

for the opposite workflow (pattern->opengraph->PYZX) do(n't) we want to check the compatibility with gflow so that we can go pat-ZX-back to pat while maintaining semantics?

I don't think this necessary in the open graph -> ZX or ZX -> open graph code. A ZX graph does not require flow. If you want to convert it to a ZX circuit (not a graph) then you will likely use the zx.extract_circuit function which will check for flow when creating the graph.

yes if it's graph <> ZX, there's no flow involved. Once we involve patterns, we need to care about flows, and what type (flow/gflow/pailiflow, max-delayed, focused, other). This is what @masa10-f has worked on, which is perhaps something we can incorporate later as a feature-rich transpilation between open graph and pattern (to be in separate PR).

Are you saying that you wish to attach information about the flow to the Open Graph? I'm not familiar with the use case here so perhaps we can coordinate more with @masa10-f later on as it definitely sounds like something we can incorporate.

separate from above, do we want to implement OpenGraph.to_pattern to wrap generate_from_graph, as well as write up bare minimum implementation of OpenGraph.from_pattern (i.e. just extarct the open graph, noting limitation of it)?

Sure. I can add a .to_pattern method. How would .from_pattern be implemented? Is there an existing function that converts patterns to the (nx.graph, in, out, meas) tuple?

[wlcsm]

I personally don't prefer to include .qasm files or even pyzx due to package quality issues. They should be replaced with self-hosted implementations, at least in the future.

I understand that it looks like an unnecessary step to hold qasm files when we are testing the PyZX graphs --- with the code the way it is now, it would be more ideal to replace the qasm with code that directly constructs the graph in the unit test.

The main reason I have included qasm is so that we can in future create tests to verify the correctness of the PyZX -> OpenGraph -> Pattern pipeline by for instance running the QASM and the resulting pattern on simulators and comparing the resulting tensors. If we directly construct the graphs using code instead, then we can't run it on an alternative simulator to compare the results.

Regarding seperating PyZX from the code. I agree that this could be a good decision so that people can use the interface and not have to install PyZX if they don't need it. @EarlMilktea @shinich1 Would it then be better to move the to_pyzx_graph and from_pyzx_graph methods into a separate file (e.g. open_graph_pyzx.py) and make them their own functions?

def to_pyzx_graph(g: OpenGraph) -> pyzx.Graph
def to_pyzx_graph(g: pyzx.Graph) -> OpenGraph

[wlcsm]

@shinich1 I moved the pyzx dependancy to requirements-dev.txt but now tox is failing in the CI because the minimum-deps-py environments don't install the dependencies in requirements-dev.txt.

Some ways we could solve this could be to either hardcode installing pyzx in the tox.ini file, or alternatively, to make the minimum-deps-py environment skip tests/test_open_graph.py. What are your thoughts?

[shinich1]

@shinich1 I moved the pyzx dependancy to requirements-dev.txt but now tox is failing in the CI because the minimum-deps-py environments don't install the dependencies in requirements-dev.txt.

Some ways we could solve this could be to either hardcode installing pyzx in the tox.ini file, or alternatively, to make the minimum-deps-py environment skip tests/test_open_graph.py. What are your thoughts?

Good catch, how about doing the follwoing: https://github.com/TeamGraphix/graphix/blob/364be19de0b1cc3a9be559eb2891f2a2e56ff6b7/tests/test_graphsim.py#L137

[shinich1]

Sure. I can add a .to_pattern method. How would .from_pattern be implemented? Is there an existing function that converts patterns to the (nx.graph, in, out, meas) tuple?

for example, https://github.com/TeamGraphix/graphix/blob/364be19de0b1cc3a9be559eb2891f2a2e56ff6b7/graphix/gflow.py#L716-L722

[wlcsm]

@shinich1 Thank you for your suggestions, it has made implementing the changes quite smooth.

I have added OpenGraph.to_pattern and OpenGraph.from_pattern methods.

Regarding the PyZX tests, the pytest.skip decorator works but because the OpenGraph.to_pyzx_graph and ``OpenGraph.from_pyzx_graph` are methods, we must import pyzx in the file containing the OpenGraph class definition, hence people wouldn't be able to import the OpenGraph without having PyZX installed.

To fix this, I have moved these two methods into a seperate file graphix/pyzx.py and their tests into a seperate file as well tests/test_pyzx.py. This way people can use OpenGraph and the OpenGraph.to_pattern methods without having PyZX installed.

[wlcsm]

@shinich1 @thierry-martinez After resolving all your comments, I have simplified the OpenGraph datastructure to simply hold the nx.Graph, inputs, outputs, and measurements separately. This is primarily because it is simpler to understand. The reason we put in all in one nx.Graph previously was actually to make some operations required by our compiler easier to perform, but I see no need here.

[shinich1]

@wlcsm thanks! one more thing, can we replace these qasm files by cliffordT generator of pyzx? I think this will improve package quality while maintaining tests

another option would be to do the same as https://github.com/TeamGraphix/graphix/blob/master/tests/random_circuit.py, with pyzx.Circuit and use it for testing

[shinich1]

@wlcsm thanks! one more thing, can we replace these qasm files by cliffordT generator of pyzx? I think this will improve package quality while maintaining tests

implemented as above, please take a look.

[wlcsm]

@wlcsm thanks! one more thing, can we replace these qasm files by cliffordT generator of pyzx? I think this will improve package quality while maintaining tests

implemented as above, please take a look.

Hi @shinich1 , thanks for proactively implementing the change! I realise I neglected to mention that I was on vacation last week and so I unfortunately wasn't quick to respond to your comments.

Your change is great implementing the randomised circuit testing is great. I changed a couple of lines to ensure the pyzx dependency is optional and removed the QASM files. So now I think it is all ready to go

[EarlMilktea]

@wlcsm Could you resolve my suggestions please?

[wlcsm]

@wlcsm Could you resolve my suggestions please?

Hey @EarlMilktea, which suggestions are referring to? I can't see any unresolved suggestions

[EarlMilktea]

@wlcsm

It might be collapsed like this:

[wlcsm]

@EarlMilktea When I click to see which changes you have requested here It takes me to this set of changes which have all been marked as resolved

It might be collapsed like this:

I've expanded the discussions and I still can't find any unresolved changes 😅 Sorry for the trouble but would you be able to provide a link to the unresolved suggestions please?

[EarlMilktea]

@wlcsm

I forgot to request changes after adding comments! :sob:

[wlcsm]

@EarlMilktea No worries 😄

We are now using randomised circuits for testing and removed the QASM circuits, so the changes were naturally resolved.

I've resolved your changes except one, please have a look at my reply

[shinich1]

@wlcsm please squash and merge!

[shinich1]

will bump pypi version soon.