Opened 6 years ago
Closed 5 years ago
#20849 closed enhancement (fixed)
Improve grs.py documentation
Reported by: | dlucas | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-7.6 |
Component: | coding theory | Keywords: | rd3 |
Cc: | jsrn | Merged in: | |
Authors: | David Lucas, Clément Pernet | Reviewers: | Daniel Augot, Travis Scrimshaw, Clément Pernet |
Report Upstream: | N/A | Work issues: | |
Branch: | 2ae9428 (Commits, GitHub, GitLab) | Commit: | 2ae9428ae8e264beb34b031be9de72291af16dd8 |
Dependencies: | Stopgaps: |
Description
Some doctests in grs.py are quite unhelpful, and some classes description do not explain at all how the class works (especially for the encoders).
This ticket fixes that.
Change History (32)
comment:1 Changed 6 years ago by
- Summary changed from Improved grs.py documentation to Improve grs.py documentation
comment:2 Changed 6 years ago by
- Branch set to u/dlucas/rewrite_grs_docstrings
comment:3 Changed 6 years ago by
- Commit set to 5dccaa08e04a65b3548bb802a6667479e975b0e3
- Status changed from new to needs_review
comment:4 Changed 6 years ago by
Hi David,
typos:
- scalaers
remarks and suggestions:
minimum_distance
the new doc string is less clear than previous one. The notion of minimum distance is clear in coding theory, no need to be explicit. If you really want to explicit it, change the sentence of the new docstring toReturns the minimum distance between any two words in
self
.
column_multipliers
,parity_column_multipliers
: the new doc strings speaks about positions scalars, while these methods deal with multipliers.
weight_enumerator
: it would be better to say "the polynomial whose coefficient of degree is [...]" to avoid naming the variable 'x'
- (several places) the framework for describing the encoding could be made a bit more clear, using the terms "message" and "encoding" more explicitly. Instead of
Let `m = (m_1, \dots, m_k)` a vector over F. [...] The corresponding codeword is the following vector:
what about
let `m=[...]` a vector over F be the message: [...] the encoding of `m` will be the following codeword:
class GRSErrorErasureDecoder(Decoder)
it would be better to say : floor((d-1)/2 instead of "decoding radius". Also the input to the decode method should be aword_and_erasure
vector, not a vectory
.
Daniel
comment:5 Changed 6 years ago by
- Status changed from needs_review to needs_work
needs rebase.
also:
- typo:
scalaers
comment:6 Changed 6 years ago by
- Branch changed from u/dlucas/rewrite_grs_docstrings to u/cpernet/rewrite_grs_docstrings
comment:7 Changed 6 years ago by
- Commit changed from 5dccaa08e04a65b3548bb802a6667479e975b0e3 to 21ce2d0d52609fe68ec28a9bbb4a0be37db7e596
Rebased and fixed the scalaers
typo. Other remarks by danielaugot still need to be adressed.
New commits:
21ce2d0 | Merge branch 'u/dlucas/rewrite_grs_docstrings' of git://trac.sagemath.org/sage into grs_docstring
|
comment:8 Changed 6 years ago by
- Keywords rd3 added
comment:9 Changed 6 years ago by
- Commit changed from 21ce2d0d52609fe68ec28a9bbb4a0be37db7e596 to b2dfd4da6ed53f6168545c82ce1d694951eab51c
Branch pushed to git repo; I updated commit sha1. New commits:
b2dfd4d | fix docstring following danielaugot's suggestions and turn verbs to imperative
|
comment:10 Changed 6 years ago by
- Status changed from needs_work to needs_review
I fixed the merge conflict and all outstanding remarks by danielaugot. I also took this opportunity to turn the verbs to imperative at the beginning of almost each docstring. Ready for review
comment:11 Changed 6 years ago by
A very small typo with maths in the docstring of GRSErrorErasureDecoder
. The hamming_weight
gives a bad rendering in math mode, just fix the "_
" in
- Create a new GRS code of length `n-hamming_weight(e)`, dimension `k`.
docstring.
comment:12 Changed 6 years ago by
- Commit changed from b2dfd4da6ed53f6168545c82ce1d694951eab51c to 20368066a5d873db5fbd3549648d846cc7bd5c7c
Branch pushed to git repo; I updated commit sha1. New commits:
2036806 | fix typesetting of Hamming weight
|
comment:13 Changed 6 years ago by
Typesetting updated.
comment:14 Changed 6 years ago by
- Reviewers set to Daniel Augot
- Status changed from needs_review to positive_review
comment:15 Changed 6 years ago by
- Status changed from positive_review to needs_work
PDF docs don't build
[docpdf] l.7167 ...code of length \(n-\text{hamming_weight} 177318[docpdf] (e)\), dimension \(k\). 177319[docpdf] ! ==> Fatal error occurred, no output PDF file produced!
comment:16 Changed 6 years ago by
- Commit changed from 20368066a5d873db5fbd3549648d846cc7bd5c7c to 1d9c6cbfe7b211b189aa11eb3f342a22f8067b59
Branch pushed to git repo; I updated commit sha1. New commits:
1d9c6cb | fix \text failing to compile in pdf.
|
comment:17 Changed 6 years ago by
- Status changed from needs_work to needs_review
Fixed. Ready for a (trivial) review.
comment:18 Changed 6 years ago by
On my fedora laptop, make doc-pdf
fails. It also fails on branch develop
, even with a fresh git clone
. Someone else than me, with a different distro, is needed to positively review the ticket.
comment:19 Changed 6 years ago by
I did a refined test. The following builds nicely the coding pdf doc.
./sage -b ./sage --docbuild reference/coding pdf
and evince ./local/share/doc/sage/pdf/en/reference/coding/coding.pdf
shows a correct pdf.
comment:20 Changed 6 years ago by
- Status changed from needs_review to positive_review
comment:21 Changed 6 years ago by
- Milestone changed from sage-7.3 to sage-7.6
I've pushed a branch to u/tscrim/improve_grs-20849
that does a little bit more cleanup of the doc (including some of the parts added here) and of the file itself. If you want to review that here, then go ahead and do so, but feel free to use that as a branch for a followup ticket too.
comment:22 follow-up: ↓ 24 Changed 6 years ago by
- Status changed from positive_review to needs_work
Travis. Thanks for your follow-up corrections. Please push your branch to this ticket, as it is based on the latest commit of mine. I am willing to positive review all of your changes except the following ones:
- in several place, you added a line break before a \textnormal{ Reed Solomon Code over }... there is an extra space identation in each of these which I think should be removed.
- You replaced
a new GRS code of length `n-hamming\_weight(e)`
by
a new GRS code of length equal to the `n`-hamming weight of `e`
.
This sentence does not make sense. Maybe change it to a new GRS code of length equal to `n` minus the Hamming weight of `e`
(move the
and capitalize Hamming
)
Hence I put the ticket in needs-work status again.
comment:23 Changed 6 years ago by
Also can you fix line 957:
an polynomial
to a polynomial
comment:24 in reply to: ↑ 22 Changed 6 years ago by
- Branch changed from u/cpernet/rewrite_grs_docstrings to u/tscrim/improve_grs-20849
- Commit changed from 1d9c6cbfe7b211b189aa11eb3f342a22f8067b59 to e9fcb770f46c3521cb0b52ef43059a6cfe68a921
- Reviewers changed from Daniel Augot to Daniel Augot, Travis Scrimshaw
Replying to cpernet:
Travis. Thanks for your follow-up corrections. Please push your branch to this ticket, as it is based on the latest commit of mine. I am willing to positive review all of your changes except the following ones:
- in several place, you added a line break before a \textnormal{ Reed Solomon Code over }... there is an extra space identation in each of these which I think should be removed.
At the risk of bikeshedding, the spaces are there in order to show that the line came from a line break in the output. This is used in a number of other places in Sage too. IMO it builds a better association to the line above it, but I don't have a strong opinion on this.
- You replaced
a new GRS code of length `n-hamming\_weight(e)`
by
a new GRS code of length equal to the `n`-hamming weight of `e`
. This sentence does not make sense. Maybe change it toa new GRS code of length equal to `n` minus the Hamming weight of `e`
(movethe
and capitalizeHamming
)
Ah, I mis-parsed that sentence. I was wondering what the n
-Hamming weight was and why I couldn't find a definition. Fixed.
New commits:
e2d07fc | Merge branch 'u/cpernet/rewrite_grs_docstrings' of git://trac.sagemath.org/sage into u/tscrim/improve_grs-20849
|
cde29f7 | Doing a little bit of extra cleanup.
|
e9fcb77 | Some details.
|
comment:25 Changed 6 years ago by
- Status changed from needs_work to needs_review
comment:26 Changed 5 years ago by
- Commit changed from e9fcb770f46c3521cb0b52ef43059a6cfe68a921 to d631689019139a0f96784efd2555f1bd8d3fd930
Branch pushed to git repo; I updated commit sha1. New commits:
d631689 | Merge branch 'develop' into u/tscrim/improve_grs-20849
|
comment:27 Changed 5 years ago by
ping
comment:28 Changed 5 years ago by
- Branch changed from u/tscrim/improve_grs-20849 to u/cpernet/improve_grs-20849
comment:29 Changed 5 years ago by
- Commit changed from d631689019139a0f96784efd2555f1bd8d3fd930 to 2ae9428ae8e264beb34b031be9de72291af16dd8
Positive review to all recent commits by Travis. I also pushed a minor edit, replacing H(e)
by the standard notation w(e)
.
Travis, can you please just review this last edit and were fine.
New commits:
521fc25 | Use the more standard notation w for Hamming weight function [Roth'06].
|
2ae9428 | Merge branch 'u/tscrim/improve_grs-20849' of git://trac.sagemath.org/sage into u/tscrim/improve_grs-20849
|
comment:30 Changed 5 years ago by
- Reviewers changed from Daniel Augot, Travis Scrimshaw to Daniel Augot, Travis Scrimshaw, Clément Pernet
comment:31 Changed 5 years ago by
- Status changed from needs_review to positive_review
LGTM. Sorry, I didn't know standard notation and just took a guess.
comment:32 Changed 5 years ago by
- Branch changed from u/cpernet/improve_grs-20849 to 2ae9428ae8e264beb34b031be9de72291af16dd8
- Resolution set to fixed
- Status changed from positive_review to closed
I pushed my branch, this is now open for review.
New commits:
Improved documentation in grs.py