Improve grs.py documentation

[sagetrac-dlucas]

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.

CC: @johanrosenkilde

Component: coding theory

Keywords: rd3

Author: David Lucas, Clément Pernet

Branch/Commit: 2ae9428

Reviewer: Daniel Augot, Travis Scrimshaw, Clément Pernet

Issue created by migration from https://trac.sagemath.org/ticket/20849

[sagetrac-dlucas]

Branch: u/dlucas/rewrite_grs_docstrings

[sagetrac-dlucas]

Commit: 5dccaa0

[sagetrac-dlucas]

comment:3

I pushed my branch, this is now open for review.

---

New commits:

5dccaa0

Improved documentation in grs.py

[sagetrac-dlucas]

Author: David Lucas

[sagetrac-danielaugot]

comment:4

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 to Returns 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 a word_and_erasure vector, not a vector y.

Daniel

[sagetrac-ylchapuy]

comment:5

needs rebase.

also:

• typo: scalaers

[ClementPernet]

Changed branch from u/dlucas/rewrite_grs_docstrings to u/cpernet/rewrite_grs_docstrings

[ClementPernet]

Changed commit from 5dccaa0 to 21ce2d0

[ClementPernet]

comment:7

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

[ClementPernet]

Changed keywords from none to rd3

[sagetrac-git]

Changed commit from 21ce2d0 to b2dfd4d

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

b2dfd4d

fix docstring following danielaugot's suggestions and turn verbs to imperative

[ClementPernet]

Changed author from David Lucas to David Lucas, Clément Pernet

[ClementPernet]

comment:10

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

[sagetrac-danielaugot]

comment:11

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.

[sagetrac-git]

Changed commit from b2dfd4d to 2036806

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

2036806

fix typesetting of Hamming weight

[ClementPernet]

comment:13

Typesetting updated.

[sagetrac-danielaugot]

Reviewer: Daniel Augot

[sagetrac-danielaugot]

comment:19

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.

[tscrim]

comment:21

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.

[ClementPernet]

comment:22

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.

[ClementPernet]

comment:23

Also can you fix line 957:
an polynomial to a polynomial

[tscrim]

comment:24

Replying to @ClementPernet:

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 to a new GRS code of length equal to `n` minus the Hamming weight of `e` (move the and capitalize Hamming)

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.

[tscrim]

Changed reviewer from Daniel Augot to Daniel Augot, Travis Scrimshaw

[tscrim]

Changed branch from u/cpernet/rewrite_grs_docstrings to u/tscrim/improve_grs-20849

[tscrim]

Changed commit from 1d9c6cb to e9fcb77

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

d631689

Merge branch 'develop' into u/tscrim/improve_grs-20849

[sagetrac-git]

Changed commit from e9fcb77 to d631689

[tscrim]

comment:27

ping

[ClementPernet]

Changed branch from u/tscrim/improve_grs-20849 to u/cpernet/improve_grs-20849

[ClementPernet]

comment:29

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

[ClementPernet]

Changed commit from d631689 to 2ae9428

[ClementPernet]

Changed reviewer from Daniel Augot, Travis Scrimshaw to Daniel Augot, Travis Scrimshaw, Clément Pernet

[tscrim]

comment:31

LGTM. Sorry, I didn't know standard notation and just took a guess.

[vbraun]

Changed branch from u/cpernet/improve_grs-20849 to 2ae9428