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

Remove diffs as internal data structure #213

Merged
merged 4 commits into from
Jan 25, 2024

Conversation

wlcsm
Copy link
Contributor

@wlcsm wlcsm commented Dec 16, 2023

We currently store the proof as both a series of graphs and as an initial graph together with a series of diffs. This PR removes the diffs to avoid possible synchronisation issues between these two structures.
As a result, it resolves issue #207

Additionally, the diffs did not record repositioned vertices, which made adding an OCM step difficult. This then opens up the way for issue #150.

The downside with this change is that it is not backwards compatible with older versions of ZXLive. I've tried to add compatibility by encoding the series of graphs as a series of diffs when saving as JSON. But I encountered problems as wasn't correctly reconstructing the same graph.

@wlcsm
Copy link
Contributor Author

wlcsm commented Dec 16, 2023

I tried encoding/decoding with diff using the simple proof where I fuse spiders 0 and 4

Screenshot 2023-12-16 at 9 24 12 pm

But when I save and open the proof, I get

Screenshot 2023-12-16 at 9 24 56 pm

It looks like a problem with computing the diff, since the proof file does contain this extra wire added between node 5 and 8

{
  "removed_verts": [
    4
  ],
  "new_verts": [],
  "removed_edges": [],
  "new_edges": [
    [
      0,
      8
    ],
    [.               <------------- Shouldn't be here
      5,
      8
    ]
  ],
  "changed_vertex_types": {},
  "changed_edge_types": {
    "0,8": 1,
    "5,8": 1
  },
  "changed_phases": {},
  "changed_pos": {},
  "changed_vdata": {},
  "variable_types": {}
}

Printing self.graph.graph in the proof panel displays the vertex map of the demo graph as

{0: {4: 1}, 1: {5: 1}, 2: {6: 1}, 3: {7: 1}, 4: {5: 1, 8: 1, 0: 1}, 5: {4: 1, 9: 1, 10: 1, 1: 1}, 6: {10: 1, 2: 1}, 7: {11: 1, 3: 1}, 8: {4: 1, 12: 1}, 9: {5: 1, 13: 2}, 10: {5: 1, 6: 1, 14: 1}, 11: {7: 1, 15: 1}, 12: {8: 1, 16: 1, 17: 1}, 13: {9: 2, 17: 2, 18: 2}, 14: {10: 1, 17: 1, 18: 1}, 15: {11: 1, 18: 1, 19: 1}, 16: {12: 1, 20: 1}, 17: {12: 1, 14: 1, 21: 1, 13: 2}, 18: {14: 1, 15: 1, 22: 1, 13: 2}, 19: {15: 1, 23: 1}, 20: {16: 1}, 21: {17: 1}, 22: {18: 1}, 23: {19: 1}}

In particular 4: {5: 1, 8: 1, 0: 1} which says that vertex 4 has an edge with node 5, which it shouldn't. Though it isn't visibly shown and its not listed in the construction of the demo graph in https://github.com/Quantomatic/zxlive/blob/9822dda6cf5d212c1372db078965d6a6cc16fb21/zxlive/construct.py#L15 . So I've not sure how it got there

@wlcsm
Copy link
Contributor Author

wlcsm commented Dec 16, 2023

My attempt to encode the proof as a series of diffs is tracked on this branch https://github.com/wlcsm/zxlive/tree/diff-encoding

@dlyongemallo
Copy link
Contributor

dlyongemallo commented Dec 17, 2023

So I've not sure how it got there

The vertices are renumbered in construct_circuit when the boundary vertices are added in these lines (note the ...+qubits, where qubits is equal to 4):

https://github.com/Quantomatic/zxlive/blob/9822dda6cf5d212c1372db078965d6a6cc16fb21/zxlive/construct.py#L28-L30

https://github.com/Quantomatic/zxlive/blob/9822dda6cf5d212c1372db078965d6a6cc16fb21/zxlive/construct.py#L38-L40

So 4: {5: 1, 8: 1, 0: 1} is correct, because it corresponds to the edges (0, 1, 0), (0, 4, 0) in (the original numbering in) elist plus the added edge (from the original vertex 0, renumbered as 4) to the added boundary vertex (numbered 0 in the new numbering system), respectively.

I was initially confused by this too when I had to debug this code earlier. I'll add a comment to clarify this.

dlyongemallo added a commit to dlyongemallo/zxlive that referenced this pull request Dec 17, 2023
The way the graph is initialised can be confusing to someone using the demo
graph to debug, since the qubit numbering used in the actual graph is shifted
by 4 from the numbering in the lists used to initialise it (due to the 4 added
input nodes).

See zxcalc#213 (comment) for context.
@wlcsm wlcsm mentioned this pull request Dec 17, 2023
@wlcsm
Copy link
Contributor Author

wlcsm commented Dec 17, 2023

The vertices are renumbered in construct_circuit when the boundary vertices are added in these lines (note the ...+qubits, where qubits is equal to 4):

Ah I see, thanks for clarifying.

I've found simply starting a derivation on the demo graph, saving without performing any steps shows the graph state as

{0: {4: 1}, 1: {5: 1}, 2: {6: 1}, 3: {7: 1}, 4: {0: 1, 5: 1, 8: 1}, 5: {1: 1, 4: 1, 9: 1, 10: 1}, 6: {2: 1, 10: 1}, 7: {3: 1, 11: 1}, 8: {4: 1, 12: 1}, 9: {5: 1, 13: 2}, 10: {5: 1, 6: 1, 14: 1}, 11: {7: 1, 15: 1}, 12: {8: 1, 16: 1, 17: 1}, 13: {9: 2, 17: 2, 18: 2}, ...}

Then after loading, it is different

{0: {16: 1, 1: 1, 4: 1}, 1: {17: 1, 0: 1, 5: 1, 6: 1}, 2: {18: 1, 6: 1}, 3: {19: 1, 7: 1}, 4: {0: 1, 8: 1}, 5: {1: 1, 9: 2}, 6: {1: 1, 2: 1, 10: 1}, 7: {3: 1, 11: 1}, 8: {4: 1, 12: 1, 13: 1}, 9: {5: 2, 13: 2, 14: 2}, 10: {6: 1, 13: 1, 14: 1}, 11: {7: 1, 14: 1, 15: 1}, 12: {8: 1, 20: 1}, 13: {8: 1, 10: 1, 21: 1, 9: 2}, ...}

Though the graph does load correct which makes me wonder if the vertex indices have been rearranged, and that is cause the diff to be applied incorrectly.

@jvdwetering
Copy link
Collaborator

I have mixed feelings about not including the diffs in the proof file. I don't have a clear reason for it, but it feels like this will bite us later. Right now the diffs are easily calculated from the graphs themselves, but I could imagine that later we might want to encode some additional information into the diffs (perhaps stuff to do with parametric phases). Anyway, if we remove it now, we could probably also add it back in later in a backwards-compatible way.

@wlcsm
Copy link
Contributor Author

wlcsm commented Dec 17, 2023

Thats a reasonable concern.
My understanding was that diffs would always contain the same information as the graphs themselves.
Could you explain a little more about what extra information you are considering adding to the diffs?

RazinShaikh

This comment was marked as duplicate.

Copy link
Collaborator

@RazinShaikh RazinShaikh left a comment

Choose a reason for hiding this comment

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

Hi @wlcsm, I recently returned from holidays and finally had a chance to go through the PR. It all looks good and I am happy to merge this.

Will Cashman and others added 3 commits January 25, 2024 20:35
Previously we stored the proof as both a series of graphs and as an
initial graph together with a series of diffs. Now we have removed the
diffs to avoid possible synchronisation issues.

Additionally, the diffs did not record repositioned vertices, which made
adding an OCM step difficult (Issue: zxcalc#150)
@RazinShaikh RazinShaikh merged commit d6fd3d4 into zxcalc:master Jan 25, 2024
2 checks passed
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.

None yet

4 participants