-
Notifications
You must be signed in to change notification settings - Fork 47
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
Bug fix to the parity_checks(ReedMuller(r, m))
along with RecursiveReedMuller
code for cross-reference
#277
Conversation
Benchmark ResultJudge resultBenchmark Report for /home/runner/work/QuantumClifford.jl/QuantumClifford.jlJob Properties
ResultsA ratio greater than
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoTarget
Baseline
Target resultBenchmark Report for /home/runner/work/QuantumClifford.jl/QuantumClifford.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfo
Baseline resultBenchmark Report for /home/runner/work/QuantumClifford.jl/QuantumClifford.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfo
Runtime information
Architecture: x86_64
Benchmark ResultJudge resultBenchmark Report for /home/runner/work/QuantumClifford.jl/QuantumClifford.jlJob Properties
ResultsA ratio greater than
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoTarget
Baseline
Target resultBenchmark Report for /home/runner/work/QuantumClifford.jl/QuantumClifford.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfo
Baseline resultBenchmark Report for /home/runner/work/QuantumClifford.jl/QuantumClifford.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfo
Runtime information
Architecture: x86_64
Benchmark ResultJudge resultBenchmark Report for /home/runner/work/QuantumClifford.jl/QuantumClifford.jlJob Properties
ResultsA ratio greater than
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfoTarget
Baseline
Target resultBenchmark Report for /home/runner/work/QuantumClifford.jl/QuantumClifford.jlJob Properties
ResultsBelow is a table of this job's results, obtained by running the benchmarks.
Benchmark Group ListHere's a list of all the benchmark groups executed by this job:
Julia versioninfo
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #277 +/- ##
==========================================
+ Coverage 82.46% 82.59% +0.13%
==========================================
Files 62 63 +1
Lines 4162 4194 +32
==========================================
+ Hits 3432 3464 +32
Misses 730 730 ☔ View full report in Codecov by Sentry. |
Please review so that we can apply the fix as soon as feasible! I have used reduced row echelon form test to compare all the cases of RecursiveReedMuller with ReedMuller,
The generator for RecursiveReedMuller is widely used as subroutine for construction of new quantum codes. Therefore, added this as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a pretty helpful set of additional implementation for cross reference. I did a quick first round of review -- there are some stylistic requests for improvement and a couple of questions on definitions and correctness checks.
I probably will be able to merge this shortly if I do not find any new issues besides what I just mentioned. Could you update the changelog so that it is ready for merge? Mention there is a bug fix to the parameter_checks matrix (if this was a public type this would have been a breaking change, so it is good to document these clearly) |
This was changelog of 244 "Implemented the classical Reed-Muller code" I think RM was not exported, so I am not sure about Public Type/Breaking change part. So, maybe Please feel free to update CHANGELOG accordingly. |
Hello @Krastanov, the quantum CSS Reed Muller code PR is ready but I am waiting for this one to be finished so that I can use the constructors introuced in this PR in next one! Please review this one so I can polish the next quantum code PR for review 😬 I am excited to submit quantum codes from classical codes finally! |
parity_checks(ReedMuller(r, m))
along with RecursiveReedMuller
code for cross-reference
Hello @Krastanov, just a small reminder that the current implementation of Thank you for the valuable feedback that helped improved the PR significantly. In this PR, I have made sure to test two different representations (binomial polynomial rep vs recursive) for cross-reference and added several correctness tests and provided complete documentation. |
I tried to rename the branch to I was following your comments that "its good to document changes clearly" so I thought let's rename the branch. 🙄 |
Removing
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regrettably, this needs a rebase after the recent rework of the test suite. Sorry for the runaround, I know this is pretty annoying.
No problem at all. Rebased. Thanks for the reminder. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks great! Sorry for the slow review. After a few minor fixes to the docstrings it should be good to merge.
G(m, r) = \begin{bmatrix} | ||
G(r, m - 1) & G(r, m - 1) \\ | ||
0 & G(r - 1, m - 1) | ||
\end{bmatrix} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
G(m, r) = \begin{bmatrix} | |
G(r, m - 1) & G(r, m - 1) \\ | |
0 & G(r - 1, m - 1) | |
\end{bmatrix} | |
G(m, r) = \\begin{bmatrix} | |
G(r, m - 1) & G(r, m - 1) \\\\ | |
0 & G(r - 1, m - 1) | |
\\end{bmatrix} |
Do these backslashes render correctly? Usually they need to be escaped. Please double check before accepting this change suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your question! I have cross verified with repos that use ```math env for a lot of math expressions. When we use this, it seems we don't use double slashes, although, if we are not using, double slashes absolutely make sense. Also, I checked - double slashes are not rendered within math environment.
Hopefully, we will see a pretty matrix with math in dev doc. I will cross verify from dev doc as well.
Co-authored-by: Stefan Krastanov <github.acc@krastanov.org>
Thank you for your helpful suggestions. P.S. The |
@Krastanov, pinging for review. |
Thank you for fixing this |
Back then, I didn't had access to this book Fundamentals of Error Correcting codes by Huffman, that's why I skipped some details about Reed-Muller codes as now I am reading it now.
I have polished this PR #244 and added more details from the literature. This provides RM a complete introduction! Tested for all the cases.
@Krastanov Kindly please handle this small PR before reviewing ShortenedMDS! Thanks!