-
Notifications
You must be signed in to change notification settings - Fork 50
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
Adding classical Bose–Chaudhuri–Hocquenghem code to ECC module #263
Conversation
Fe-r-oz
commented
Apr 19, 2024
- Adding classical Bose–Chaudhuri–Hocquenghem code to ECC module
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
|
Thanks for your comments. I have significantly improved the doc-strings and made 1 commit/push. I hope you like the descent from Finite Fields back to the Boolean Field when reviewing! |
Results:
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #263 +/- ##
==========================================
- Coverage 82.28% 82.25% -0.03%
==========================================
Files 58 59 +1
Lines 3894 3940 +46
==========================================
+ Hits 3204 3241 +37
- Misses 690 699 +9 ☔ View full report in Codecov by Sentry. |
@Krastanov, Could you please help review the open PRs? Thanks. |
BCH code (15, 2) Parity Check Matrix: H
HField Matrix:
julia> GF2, a = finite_field(2, 4, "a")
|
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 setting this up! I finished a first round of a review. There are some superficial comments, but also a bunch of fairly important comments related to testing and error messages.
I also tried to run a few examples, and I am getting some strange results for some parameter regimes. E.g. the following:
julia> parity_checks(BCH(7,4)) |> size
(12, 7)
This does not make sense to me. A parity check matrices should have fewer rows than columns. This should probably be explicitly part of some form of universal tests -- the rank of the matrix should be equal to the number of rows it has which should be smaller than the number of columns, for any code.
This should be part of the tests, not just a comment on the pull requests (it is one of the todos I have listed in the review). Make sure these tests have references to where the example is taken from. |
I made a mistake in the setting the bound of t. After reading the slides, the m >= 3 and t < 2^(m -1 ). n = Block length: n = 2^m − 1. When m =3, we have n=7, and GF(2^3). When n =7 is actually 2^3 - 1, GF(8), so m = 3. Now, t must be < 2^(3 - 1) so t < 4. (7, 4) is not a valid instance of the code and the bound is set incorrectly. I have set it to t <= 2^(m - 1) so that why the invalid instance (7, 4) is not throwing the error. So, the m =3, the valid codes are (7, 1), (7, 2) and (7, 3) as t should be less than 2^(m - 1) as per the formula!
|
Thanks for comprehensive review. I have incorporated all your suggestions.
|
Co-authored-by: Stefan Krastanov <[email protected]>
Co-authored-by: Stefan Krastanov <[email protected]>
Co-authored-by: Stefan Krastanov <[email protected]>
Co-authored-by: Stefan Krastanov <[email protected]>
…nto BCHbash: ^X: command not found
Thanks for your comments. Added all your suggestions, also downgrade is passing now 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.
Looks good! I will make a few small changes (check the comments and the upcoming commits) and if tests pass I will merge.
You had mentioned that in some situations tests take long time. A convenient workaround that does not sacrifice the quality of the tests is to do something like:
for t in rand(all_test_cases, n) # where n is some small number of samples, e.g. 5 or 10
instead of
for t in all_test_cases
That way everything gets tested eventually but you do not need to worry about things being too expensive each time.
Of course, that should be done only after someone has actually executed all the tests at least once, e.g. during developing the package.
Thanks for taking care of this submission. It is quite polished now.
…Conjugates" because I do not understand its purpose but I also do not want to delay the merge, minor rework on tests and references
I have followed your docstring documentation style and added a small and relevant paragraph about conjugates. I hope this that satisfies you. This will save your time in for docstring. Also, the last check is now done via formula. Apparently, I forgot to change the page numbers from 202, 203 to 168, 169. During tighting of the new docstring paragraph, please change the page numbers as well in the test section. |
Fixed the page numbers as well. I have polished this up as well! |
This is great! I removed a minor part of the docstring that I do not understand well. I do not want to keep you waiting on a merge over such a minor thing, given that the rest seems already very polished. If you consider that part of the docstring important, we can discuss it over a separate pull request. Do you want me to start reviewing a specific next pull request? I see you have submitted a few, but I am not sure which one you would like to start with. |
Woohoo! |
Please review Reed-Solomon next! |