zxcalc / pyzx

Python library for quantum circuit rewriting and optimisation using the ZX-calculus
Apache License 2.0
365 stars 108 forks source link

Optimisation of rz and rx circuits sometimes unsound. #9

Closed rossduncan closed 4 years ago

rossduncan commented 5 years ago

This took a lot of compute time but :

  1. loading in a large qasm file
  2. applying a lot of optimisations
  3. compare_tensors returns False.

See attached files.

Archive.zip

jvdwetering commented 5 years ago

Thank you for the report. I think the error is that zx.optimize.phase_block_optimize doesn't yet correctly deal with phases that aren't multiples of pi/4. I'm afraid I can't verify myself since my laptop runs out of memory using compare_tensors. In any case, phase_block_optimize will not do much anyway when not dealing with Clifford+T circuits, so I would advise not using that for the arbitrary rotations that you are using.

wo3kie commented 5 years ago

Hi,

For this input

OPENQASM 2.0;
include "qelib1.inc";

qreg q[128];

ccx q[124], q[125], q[126];
cx q[126], q[27];
ccx q[124], q[125], q[126];

I am getting this output

Traceback (most recent call last):
  File "./pyzx_test.py", line 11, in <module>
    print(new_circuit.to_qasm())
  File "/mnt/c/Users/Lenovo/source/pyzx/pyzx/circuit.py", line 368, in to_qasm
    s += g.to_qasm() + "\n"
  File "/mnt/c/Users/Lenovo/source/pyzx/pyzx/circuit.py", line 852, in to_qasm
    raise TypeError("Gate {} doesn't have a QASM description".format(str(self)))
TypeError: Gate ParityPhase(7/4, 126, 124) doesn't have a QASM description

Have I missed something? Łukasz

jvdwetering commented 5 years ago

Try changing the line to new_circuit.to_basic_gates().to_qasm()

lia-approves commented 4 years ago

Hello, here are a few far smaller test cases for which compare_tensor fails post-optimization. Can I ask if we're (myself and @edasgupta) making the wrong function calls?

While looking into it, we noticed that streaming_extract does not always extract the complete graph the first time. It didn't solve our issue, but we've written a hasty nested for-while-for loop so that it does --- here is a test case where it makes a difference.

jvdwetering commented 4 years ago

Hmm, that is indeed wrong behaviour. I'm gonna try to see if I can fix it. Thanks for letting me know! In case you need something now, I've implemented a new modified_extract function in pyzx.extract which should be able to extract the same stuff as streaming_extract. It is not yet super well tested but it in any case seems to work on the example you gave above.

lia-approves commented 4 years ago

That sounds great. What does modified_extract do? We don't see the function on the repo yet, would you be able to push your branch?

Thanks so much for getting back to us! We're summer interns at IBM - for these past 3 weeks we implemented a parser for Qiskit quantum circuit to PyZX and back, and are benchmarking it. We're really interested in the ZX-calculus; thank you for making this library.

jvdwetering commented 4 years ago

I think I've found the error. It was that g.normalise() didn't always do the right thing on very small graphs, and since streaming_extract() expects the graph to be normalized, it wouldn't do the right thing. It should work now.

modified_extract is a simpler algorithm based on a new insight into the extraction problem. More details about how it works should appear in a paper pretty soon. I've just noticed that it does some very inefficient things when applied to these small circuits, so I would now actually advice against using it.

Thank you for working on PyZX! If you have something that you would like to have merged into the main branch just let me know :)

lia-approves commented 4 years ago

Thanks for the fix. We tested on 1000 random small circuits and as a result, twice as many passed the compare_tensor test this time around. We're still failing 3/4 of the time. Can we ask if you got the smaller test cases linked above to pass?

jvdwetering commented 4 years ago

Sorry it took me a while to respond to this. I see you are using compare_tensors(c,c2) instead of compare_tensors(c,c2,False) which means the comparison is sensitive to global phases. The circuits should have been nevertheless equal, so I'm not exactly sure where a global phase is introduced. In any case, I'm assuming you don't care about global phases, so you should just use compare_tensors(c,c2,False). Get back at me if there are still comparisons that fail then.

jvdwetering commented 4 years ago

Thinking about it, it might have to do with a Hadamard gate and its Euler decomposition only being equal up to global phase. Since the extraction method doesn't carry scalars trough, this information is lost.

jvdwetering commented 4 years ago

As this is a really old issue that is probably no longer relevant, I'm gonna close it.