Skip to content
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

Add Open graph class #191

Merged
merged 40 commits into from
Aug 23, 2024
Merged

Add Open graph class #191

merged 40 commits into from
Aug 23, 2024

Conversation

wlcsm
Copy link
Collaborator

@wlcsm wlcsm commented Jul 30, 2024

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.

This is based on the Open Graph definition from the "Picturing Quantum
Software" textbook.
Comes with methods to convert to and from pyzx diagrams.

Provides properties inputs(), outputs(), and measurements() that are
compatible with the rest of graphix.
Copy link

codecov bot commented Jul 30, 2024

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 ⚠️
graphix/pyzx.py 90.24% 8 Missing ⚠️
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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

graphix/open_graph.py Outdated Show resolved Hide resolved
tests/test_open_graph.py Outdated Show resolved Hide resolved
requirements.txt Outdated Show resolved Hide resolved
graphix/open_graph.py Outdated Show resolved Hide resolved
graphix/open_graph.py Outdated Show resolved Hide resolved
graphix/open_graph.py Outdated Show resolved Hide resolved
graphix/open_graph.py Outdated Show resolved Hide resolved
tests/test_open_graph.py Outdated Show resolved Hide resolved
@wlcsm wlcsm force-pushed the opengraph branch 2 times, most recently from 60353a7 to ac74914 Compare August 2, 2024 17:24
@wlcsm
Copy link
Collaborator Author

wlcsm commented Aug 2, 2024

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?

graphix/open_graph.py Outdated Show resolved Hide resolved
graphix/open_graph.py Outdated Show resolved Hide resolved
graphix/open_graph.py Outdated Show resolved Hide resolved
graphix/open_graph.py Outdated Show resolved Hide resolved
@shinich1
Copy link
Contributor

shinich1 commented Aug 2, 2024

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
Copy link
Collaborator Author

wlcsm commented Aug 3, 2024

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.

graphix/open_graph.py Outdated Show resolved Hide resolved
@shinich1
Copy link
Contributor

shinich1 commented Aug 3, 2024

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
Copy link
Contributor

shinich1 commented Aug 3, 2024

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
Copy link
Contributor

@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
Copy link
Collaborator Author

wlcsm commented Aug 3, 2024

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
Copy link
Collaborator Author

wlcsm commented Aug 3, 2024

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
Copy link
Collaborator Author

wlcsm commented Aug 18, 2024

@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
Copy link
Contributor

@wlcsm Could you resolve my suggestions please?

@wlcsm
Copy link
Collaborator Author

wlcsm commented Aug 19, 2024

@wlcsm Could you resolve my suggestions please?

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

@EarlMilktea
Copy link
Contributor

@wlcsm

It might be collapsed like this:

collapse

@wlcsm
Copy link
Collaborator Author

wlcsm commented Aug 19, 2024

@EarlMilktea When I click to see which changes you have requested here
Screenshot 2024-08-19 at 10 37 17 PM
It takes me to this set of changes which have all been marked as resolved
Screenshot 2024-08-19 at 10 36 42 PM

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?

tests/test_pyzx.py Outdated Show resolved Hide resolved
tests/test_pyzx.py Outdated Show resolved Hide resolved
tests/test_pyzx.py Outdated Show resolved Hide resolved
tests/test_pyzx.py Outdated Show resolved Hide resolved
graphix/pyzx.py Outdated Show resolved Hide resolved
graphix/pyzx.py Outdated Show resolved Hide resolved
tests/test_pyzx.py Show resolved Hide resolved
graphix/opengraph.py Outdated Show resolved Hide resolved
tests/test_pyzx.py Outdated Show resolved Hide resolved
tests/test_pyzx.py Outdated Show resolved Hide resolved
@EarlMilktea
Copy link
Contributor

@wlcsm

I forgot to request changes after adding comments! 😭

@wlcsm
Copy link
Collaborator Author

wlcsm commented Aug 20, 2024

@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

Copy link
Contributor

@EarlMilktea EarlMilktea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!
Only one simple suggestion.

Sorry I was checking outdated codes...

@shinich1
Copy link
Contributor

@wlcsm please squash and merge!

@wlcsm wlcsm merged commit aec54c0 into master Aug 23, 2024
21 checks passed
@wlcsm wlcsm deleted the opengraph branch August 23, 2024 07:55
@shinich1
Copy link
Contributor

will bump pypi version soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants