Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Bicycle and Unicycle codes: Fixing doc and method errors #425
base: master
Are you sure you want to change the base?
Bicycle and Unicycle codes: Fixing doc and method errors #425
Changes from all commits
2c4cb47
92f6eeb
1c0fcdb
f77b135
6c3434c
077e287
a115ef5
81773e3
cac4133
2eb3eb4
49046fe
f86cd86
bd823ef
295ed67
b64baa3
3a374d8
1fe07f4
925009d
c1bba5b
b79e3ee
6e31250
94caf05
7037b76
82d19f8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I think my main remaining request for change is related to the distinction between a structure and a function.
An instance of the type
Bicycle
should be a fully defined code where quickly and deterministically one can get a parity check matrix. The "quick" part is nice to have, but "deterministic" is mandatory.Here, I think this is not deterministic. On the other hand the output of
bicycle_set_gen
seems to be all you need to be able to deterministically do the other steps. Thus I would suggest havingset
be an entry here, basically the way you have already done it forUnicycle
.That way both
Bicycle
andUnicycle
will be fully defined codes by their fields, without any randomness. And the documentation can spell out the difference in whatset
needs to be. ForBicycle
it needs to just be a set of integers (right? or maybe there are more constraints) while forUnicycle
it has to be perfect difference set?And in the docstring you can mention "we have a heuristic for finding useful sets in the function
yadayada
"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.
Is this something you invented or something from the paper? In either case, explain why it is needed. If it is from the paper, give a specific reference where in the paper it is.
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.
The
convert(Array,
is pretty suspicious. Do you need it?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.
same question about where this is coming from and why is it necessary
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 an n^4 algorithm, right? n^3 for the Gaussian elimination in
rank
and then another factor of n for the loop?Add something to the documentation saying that "a naive implementation of row-reduction currently bottlenecks the use on very large codes".
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.
same question about provenance and invention and reference
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.
if this is your own thing, you should brag about it a bit more loudly :D
Check warning on line 242 in src/ecc/codes/simple_sparse_codes.jl
GitHub Actions / Spell Check with Typos