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

adding classical Reed-Muller Code #244

Merged
merged 12 commits into from
Mar 22, 2024
Merged

Conversation

Fe-r-oz
Copy link
Contributor

@Fe-r-oz Fe-r-oz commented Mar 20, 2024

Prof. Stefan,

Adding Classical Reed-Muller Code, RM (r, m ). This code will be the second addition after RepCode which is a important subroutine in already implemented codes such as Toric.

Reed Muller Code will have a similar objective, as classical reed muller codes can be used to create quantum codes.

While updating the changelog, I found something odd. It has **(breaking) and **(fix) appears in the main master branch. Please see to it.

Added Test Throws as well.

I tested locally as well.

Reed-Muller Code Parameters: ReedMuller(2, 3)
Parity Matrix:
[1 1 1 1 1 1 1 1; 1 1 1 1 0 0 0 0; 1 1 0 0 1 1 0 0; 1 0 1 0 1 0 1 0; 1 1 0 0 0 0 0 0; 1 0 1 0 0 0 0 0; 1 0 0 0 1 0 0 0]

Reed-Muller Code Parameters: ReedMuller(1, 3)
Parity Matrix:
[1 1 1 1 1 1 1 1; 1 1 1 1 0 0 0 0; 1 1 0 0 1 1 0 0; 1 0 1 0 1 0 1 0] 

@Krastanov
Copy link
Member

This is great! Thank you! I will leave some superficial comments today, just on style or more obvious things. I will try to review the more sophisticated math content later

Copy link
Member

@Krastanov Krastanov left a comment

Choose a reason for hiding this comment

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

Some minor style comments, nothing particularly significant.

It needs tests.

Could you let me know what you used as a reference when implementing this?

While updating the changelog, I found something odd. It has **(breaking) and **(fix) appears in the main master branch. Please see to it.

I do not see anything, let me know if I have missed something.

src/ecc/ECC.jl Outdated Show resolved Hide resolved
src/ecc/codes/reedmullercode.jl Outdated Show resolved Hide resolved
src/ecc/codes/reedmullercode.jl Outdated Show resolved Hide resolved
test/test_throws.jl Outdated Show resolved Hide resolved
src/ecc/ECC.jl Outdated Show resolved Hide resolved
@Fe-r-oz
Copy link
Contributor Author

Fe-r-oz commented Mar 20, 2024

This is great! Thank you! I will leave some superficial comments today, just on style or more obvious things. I will try to review the more sophisticated math content later

I verified the results of R(2,3) and R(1,3) from wiki: They have the complete matrices

As for the logic and how we generate these codes, here is a proper reference from Google Scholar. This has been very helpful as it explained them very simply. This might not be the reference of person who invented them, but it was very helpful for understanding.

@Fe-r-oz
Copy link
Contributor Author

Fe-r-oz commented Mar 20, 2024

It needs tests.

I will create a test file. I was looking where were Rep Code tests were defined, but as you pointed out, I will tests all the functions in a new test file.

As for changelog, I see this breaking at some lines,. This is just a general question, not related to #244 though.

Screenshot_select-area_20240321015410

@Krastanov
Copy link
Member

on "breaking": The changelog is there to tell other people about what has changed so they do not need to look at the git history. Last night I released version 0.9.0 (a "breaking" release as defined in SemVer https://semver.org/ ) and so the changelog has these breaking changes clearly marked up so that users know if they need to change something about their use of the library.

on references: Add that paper to the bibtex and reference it in the docstring. Also, due to a current limitation of how bibtex references are managed, add a mention of it in the list at the bottom of references.md

on tests: yeah, it is possible that test_throws.jl contains things that would have been better put in a different file. This will happen frequently, situations where someone reviews your code and asks you to do something to a better standard than what is already done in the library. It is the "leave a place better than you found it" principle, and it is the only way to not devolve into a mess, but it can be understandably annoying to the new contributor.

When you add the tests, mark in comments where the "correct" results in the comparisons are coming from.

@Fe-r-oz
Copy link
Contributor Author

Fe-r-oz commented Mar 21, 2024

For RM (r,m) How the generator matrix is created is on Page 5. This is the main reference used.

ECC Zoo and paper main cited paper used the notation RM(r,m). Some paper uses RM(m,r).

I have tested the code against all available RM(r,m) constructions available in Generator Matrix Form. The order of some rows change like as in Gottesman, but the matrices match exactly.

Agorithm used is not homegrown. Standard reference from Reference

@Krastanov
Copy link
Member

I mentioned this before but just a reminder: do not send a ton of small commits one after the other. That causes a ton of emails and a ton of expensive test runs on unfinished code. Do a push when you have something ready.

@Fe-r-oz
Copy link
Contributor Author

Fe-r-oz commented Mar 21, 2024

I mentioned this before but just a reminder: do not send a ton of small commits one after the other.

Sorry for that. I did send the first commit will most files. Actually, I missed some changes.

docs/src/references.bib Outdated Show resolved Hide resolved
docs/src/references.bib Outdated Show resolved Hide resolved
@Fe-r-oz
Copy link
Contributor Author

Fe-r-oz commented Mar 21, 2024

Improvements as follows:

  1. used unexported symbol for RM (r, m) rather than exporting it.
  2. add the original references by Muller and Reed from 1954 Papers of IEEE Transactions on Information Theory.
  3. added the Rank Test for RM (r, m) codes.
  4. added Google Scholar citations for all references

Sorry, it took 3 commits instead of 1, but all suggested changes are completed,

@Krastanov
Copy link
Member

you do not need to make a single commit, but please make sure you are making a single push, not a ton of frequent pushes

@Krastanov
Copy link
Member

This looks really great, thank you for contributing it! I have one very minor comment, but otherwise this seems ready to merge.

@Fe-r-oz
Copy link
Contributor Author

Fe-r-oz commented Mar 22, 2024

This looks really great, thank you for contributing it!

Thank you! I learned a lot as well!

@Krastanov
Copy link
Member

there was another thing I fixed in the last commit. You were calling collect, which can be an extremely expensive operation. Directly calling length is cleaner. I will wait for the tests and merge. Thank you!

@Krastanov
Copy link
Member

Thank you for the addition!

@Krastanov Krastanov merged commit 7be5818 into QuantumSavory:master Mar 22, 2024
9 of 13 checks passed
Fe-r-oz pushed a commit to Fe-r-oz/QuantumClifford.jl that referenced this pull request May 18, 2024
@Fe-r-oz Fe-r-oz deleted the RM branch July 17, 2024 17:40
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