Skip to content
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

ZXW Diagram implementation #92

Merged
merged 26 commits into from
Jul 2, 2023
Merged

Conversation

exAClior
Copy link
Collaborator

Implement ZXWDiagram.
Moving functions under zx_diagram.jl to utils.jl for reusing them for ZXWDiagram.

@codecov
Copy link

codecov bot commented Jun 18, 2023

Codecov Report

Patch coverage: 85.67% and project coverage change: -11.46 ⚠️

Comparison is base (bbc19ab) 94.34% compared to head (1f5787b) 82.88%.

Additional details and impacted files
@@             Coverage Diff             @@
##           master      #92       +/-   ##
===========================================
- Coverage   94.34%   82.88%   -11.46%     
===========================================
  Files          11       16        +5     
  Lines        2085     2478      +393     
===========================================
+ Hits         1967     2054       +87     
- Misses        118      424      +306     
Impacted Files Coverage Δ
src/ZXCalculus.jl 100.00% <ø> (ø)
src/phase.jl 74.24% <50.00%> (-8.58%) ⬇️
src/utils.jl 75.77% <75.77%> (ø)
src/zxw_diagram.jl 86.20% <86.20%> (ø)
src/parameter.jl 96.34% <96.34%> (ø)
src/to_eincode.jl 98.78% <98.78%> (ø)
src/scalar.jl 100.00% <100.00%> (ø)

... and 7 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@ChenZhao44
Copy link
Member

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.

@exAClior
Copy link
Collaborator Author

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.

This makes much more sense. I will change it accordingly. Thank you!

@exAClior
Copy link
Collaborator Author

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.

@ChenZhao44
Copy link
Member

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.

@exAClior
Copy link
Collaborator Author

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.

@exAClior
Copy link
Collaborator Author

exAClior commented Jul 1, 2023

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.

@exAClior exAClior marked this pull request as ready for review July 1, 2023 09:17
@exAClior exAClior requested a review from ChenZhao44 July 1, 2023 09:18
@exAClior exAClior changed the title [WIP] ZXW Diagram implementation ZXW Diagram implementation Jul 1, 2023
Copy link
Member

@ChenZhao44 ChenZhao44 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR looks great. I only have two minor suggestions. Once you fix this, you can merge it.

(PiUnit(pu1, _), PiUnit(pu2, _)) && if pu1 isa Number && pu2 isa Number
end => Parameter(Val(:PiUnit), pu1 + pu2)
(PiUnit(pu1, _), PiUnit(pu2, _)) && if !(pu1 isa Number) || !(pu2 isa Number)
end => Parameter(Val(:PiUnit), Expr(:call, :+, pu1, pu2))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much! I corrected the Expr part. For the pattern matched above that, I declared the constructor of Parameter as a parametric method. The type of pu should be automatically matched like in the previous implementation link The test passed alright, I will merge the changes for now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My apologies, I just realized that I didn't push my last change to the branch before merging. I will add those changes to next branch's update.

end => Parameter(Val(:PiUnit), pu1 - pu2)
(PiUnit(pu1, pu_t1), PiUnit(pu2, pu_t2)) &&
if !(pu1 isa Number) || !(pu2 isa Number)
end => Parameter(Val(:PiUnit), Expr(:call, :-, pu1, pu2))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed here too.

@exAClior exAClior merged commit 5161a51 into QuantumBFS:master Jul 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants