ZXCalculus.jl icon indicating copy to clipboard operation
ZXCalculus.jl copied to clipboard

Some feedbacks

Open GiggleLiu opened this issue 5 years ago • 1 comments

  • [ ] simplify!(Rule{:lc}(), ::ZXDiagram) should not work, but it does not give an error.
  • [x] the _ctrl in push_ctrl_gate! looks not necessary.
  • [ ] the naming of simplify! and replace! are not intuitive, should be something like simplify_recursive! and simplify_once!?
  • [ ] it would be nice to have a pipeline interface zxg |> srule!(:lc) |> srule!(:p1) |> srule_once!(:pab)
  • [ ] the following code looks confusing to me
  • [ ] do you mind changing name of phase to e.g. getphase`, because it conflicts with the phase gate in Yao.
julia> zxd = ZXDiagram(4);

julia> push_gate!(zxd, Val(:Z), 4);

julia> push_gate!(zxd, Val(:X), 4)
ZX-diagram with 10 vertices and 6 multiple edges:
(S_1{input} <-1-> S_2{output})
(S_3{input} <-1-> S_4{output})
(S_5{input} <-1-> S_6{output})
(S_7{input} <-1-> S_9{phase = 0//1⋅π})
(S_8{output} <-1-> S_10{phase = 0//1⋅π})
(S_9{phase = 0//1⋅π} <-1-> S_10{phase = 0//1⋅π})

X gate is push_gate!(zxd, Val(:X), 4, 0//1), Z gate is push_gate!(zxd, Val(:Z), 4, π) Maybe just forbid this interface?

GiggleLiu avatar Aug 29 '20 05:08 GiggleLiu

simplify!(Rule{:lc}(), ::ZXDiagram) should not work, but it does not give an error.

@ChenZhao44 define the method as:

simplify!(Rule{:lc}(), ::ZXDiagram) = error("cannot perform lc rule on ZX diagram")

would solve the issue, since simplify! implements the generic one.

the _ctrl in push_ctrl_gate! looks not necessary.

yeah, we need to rename this at some point.

the naming of simplify! and replace! are not intuitive, should be something like simplify_recursive! and simplify_once!?

simplify! and replace! has its convention in general symbolic computation/term rewrite packages cross Julia and literature. simpliy always mean recursively rewrite, and replace means substitution (or it's called substitution). But I think the doc string is very vague @ChenZhao44 we should improve doc string to mention this difference and mention something like

See also [`simplify!`](@ref).

On the other hand, we will need to integrate with SymbolicUtils at one point, since they will provide some graph match implementations, and we can serve as an simplification/contraction engine for other packages later.

it would be nice to have a pipeline interface zxg |> srule!(:lc) |> srule!(:p1) |> srule_once!(:pab)

I think just letting simplify! and replace! to have a curried version is sufficient. Also note, we don't actually have much use case for single pass simplification. The simplification algorithm always have a list of rules to apply.

We should have a list-like simplification interface at some point, tho. current methods like clifford_simplification is not necessary actually, this also as a result to have duplicated code in phase_teleport. But I suggest to postpone this to polish later.

Roger-luo avatar Aug 29 '20 13:08 Roger-luo