Closed exAClior closed 1 year ago
Patch coverage: 85.67
% and project coverage change: -11.46
:warning:
Comparison is base (
bbc19ab
) 94.34% compared to head (1f5787b
) 82.88%.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.
Thank you for the pull request; it looks great overall. I have a few minor suggestions. It seems that you have extended the SpiderType
and used it in the ZXWDiagram
. This extension also makes the original ZXDiagram
capable of handling ZXW, resulting in no difference between the new ZXWDiagram
. I suggest defining a new spider type for ZXWDiagram
only to avoid affecting the previous data structures and algorithms.
Thank you for the pull request; it looks great overall. I have a few minor suggestions. It seems that you have extended the
SpiderType
and used it in theZXWDiagram
. This extension also makes the originalZXDiagram
capable of handling ZXW, resulting in no difference between the newZXWDiagram
. I suggest defining a new spider type forZXWDiagram
only to avoid affecting the previous data structures and algorithms.
This makes much more sense. I will change it accordingly. Thank you!
Since we are implementing ZXWDiagram
with as little coupling with the original implementation with ZXDiagram
as possible, I want to take this opportunity to remove redundant data fields in ZXWDiagram
. For example, in #77 and #72, it was suggested that ZXLayout
is not necessary. I agree with this point and went ahead to remove ZXLayout
from ZXWDiagram
for now. @ChenZhao44 please let me know if this raises a concern and I would be happy to add those features back.
Since we want to separate the original implementation, we can also use algebraic data types to represent the node type and its parameters. An algebraic data type is a type that combines several data types in a type-stable way. An implementation of ADTs can be found in Expronicon. By using ADT pattern matching, as implemented in MLStyle, we can replace the if else
statements during matching calculus rules in an elegant way. We can discuss this in more detail during our next meeting.
The latest commits on ADT of Parameters
used to represent parameters for spider didn't pass the CI because YaoHIR
and CompilerPluginTools
are restricting Expronicon
to be 0.6.0
. I have already made a PR in YaoHIR
to partially solve the problem. I am temporarily removing CompilerPluginTools
from Project.toml
since Roger said he wants to rewrite CompilerPluginTools
instead of fixing it.
Hello, @ChenZhao44 I have finished correcting for ZXWDiagram
, and Parameter
and copied from your branch the implementation of converting a ZXWDiagram
to eincode
and then to Matrix
. I am waiting for @Roger-luo 's comment on my edits on CompilerPluginTools
. For the sake of passing CI, I have removed all tests related to CompilerPluginTools
.
Implement
ZXWDiagram
. Moving functions underzx_diagram.jl
toutils.jl
for reusing them forZXWDiagram
.