-
Notifications
You must be signed in to change notification settings - Fork 21
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
Conversation
I tried encoding/decoding with diff using the simple proof where I fuse spiders 0 and 4 But when I save and open the proof, I get It looks like a problem with computing the diff, since the proof file does contain this extra wire added between node 5 and 8
Printing
In particular |
My attempt to encode the proof as a series of diffs is tracked on this branch https://github.com/wlcsm/zxlive/tree/diff-encoding |
The vertices are renumbered in So I was initially confused by this too when I had to debug this code earlier. I'll add a comment to clarify this. |
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.
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
Then after loading, it is different
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. |
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. |
Thats a reasonable concern. |
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.
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.
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)
c044566
to
bf9fcb6
Compare
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.