search

Quantomatic / zxlive

A graphical tool for the ZX calculus
Apache License 2.0
44 stars 14 forks source link

Long proofs unable to load #229

Open Aerylia opened 4 months ago

Aerylia commented 4 months ago

I have created a bunch of long proofs and most of them are unable to load. I get this message: image The number changes for different files.

No errors from the terminal.

I am on Macbook Air M1, MacOS Sonoma 14.2.1

RazinShaikh commented 4 months ago

Hi @Aerylia, I just merged the pull request #213 by @wlcsm which changes the file format of the proof file. I believe that should fix the issue. Could you please try and check if it works for you now? Note that the file format is not backward compatible so you need to save your proofs again.

Aerylia commented 4 months ago

Hi, I am now getting a different error:

image

RazinShaikh commented 4 months ago

have you saved the proof again or is this an old file? Can you attach the proof file here?

Aerylia commented 4 months ago

It is the old file. I don't have the proof open. I saved it to update my OS. The problem is that I would like to work on the proof, but I cannot load it anymore. If it was open, there would be no problem.

Below the file. I have changed the extension to .txt so that Github doesn't complain. QAOA derivation1.txt

Aerylia commented 4 months ago

But if you can tell me which version of zxlive I need to open this file, I'll pull an older version. I needed these diagrams tonight...

RazinShaikh commented 4 months ago

ok let me try to debug and see if we can recover the proof from that file

dlyongemallo commented 4 months ago

Isn't this just issue #207 (which was caused by PR #204)? I think PR #204 should be reverted. It's supposed to fix saving proofs with parametrized spiders, but it makes it so that proofs can't be loaded at all (as the vertices are erroneously mislabeled after the first proof step), which is arguably worse.

As long as the proofs internally save the graphs as JSON rather than Tikz format, the saving/loading bug will happen.

dlyongemallo commented 4 months ago

I guess ZXLive is pre-release or beta software so there's no guarantee that file formats won't change, but it would still be good if the changes are better documented and users given more warning about such changes. In PR #222, I added a test which makes it so that any PR which is backwards incompatible have to explicitly update the test.

I think in general any PR which breaks backwards-compatibility should say so very clearly in the commit's message and PR description. In some other projects I've worked on, for example, there's a policy that the commit message for such changes should be, e.g., "[BREAKING CHANGE] save proofs as JSON rather than Tikz format", or something like that. Then it's clear to anyone looking through the commits to understand why something they expected to work isn't working any more.

Aerylia commented 4 months ago

As additional information: It's not about a change of file formats. I used the same version to save as I used to load and they were incompatible. Additionally, I was unable to add parameterized gates to my diagrams because they were not a fraction of pi.

RazinShaikh commented 4 months ago

Does this happen on the latest version?

dlyongemallo commented 4 months ago

In particular, please make sure you have the latest version of both ZXLive and PyZX. In your zxlive directory/virtual environment, run pip install -r requirements.txt to update to the latest changes in pyzx on GitHub.

wlcsm commented 4 months ago

I guess ZXLive is pre-release or beta software so there's no guarantee that file formats won't change, but it would still be good if the changes are better documented and users given more warning about such changes. In PR #222, I added a test which makes it so that any PR which is backwards incompatible have to explicitly update the test.

I think in general any PR which breaks backwards-compatibility should say so very clearly in the commit's message and PR description. In some other projects I've worked on, for example, there's a policy that the commit message for such changes should be, e.g., "[BREAKING CHANGE] save proofs as JSON rather than Tikz format", or something like that. Then it's clear to anyone looking through the commits to understand why something they expected to work isn't working any more.

I agree that we should better label backwards compatible changes. One way we could encourage this would be to use an PR template that asks whether the changes are backward compatible with a reminder to label them as so in the commit message. The reviewers can also verify the commit messages are appropriately labelled.

The test you added is extremely useful! I would also like to suggest we add a version field into the proofs containing the ZXLive version that was used to save the proof/diagram to aid with debugging. It could also be used to inform the user which version to switch to in the case of an error which will speed up resolution time for users who aren't able to inspect the git history.

wlcsm commented 4 months ago

Isn't this just issue #207 (which was caused by PR #204)? I think PR #204 should be reverted. It's supposed to fix saving proofs with parametrized spiders, but it makes it so that proofs can't be loaded at all (as the vertices are erroneously mislabeled after the first proof step), which is arguably worse.

As long as the proofs internally save the graphs as JSON rather than Tikz format, the saving/loading bug will happen.

I'm in favour of reverting the change as well. JSON serialisation is pyxz doesn't preserve the ordering of vertices and I currently have no quick fix for it. Though it is something I intend of resolving in the short term, testing the change may prove to take up the most time

wlcsm commented 4 months ago

@Aerylia Could you provide a screenshot of the proof or a simplified example? Since the file you provided can't be loaded, I don't know how to replicate the bug.

wlcsm commented 4 months ago

Additionally, I was unable to add parameterized gates to my diagrams because they were not a fraction of pi.

We currently don't support this. Our logic for parameterised gates is very rudimentary. I have recorded this feature request in #232

RazinShaikh commented 4 months ago

I think we already support this though. If it doesn't work, then it must be some sort of bug

dlyongemallo commented 3 months ago

Additionally, I was unable to add parameterized gates to my diagrams because they were not a fraction of pi.

Can you clarify this? Do you mean that you want the parameter to be some value a, whose value is, e.g., 0.5, and you actually want the spider to have a phase of 0.5 and not 0.5 * pi?