Opened 6 years ago

Closed 6 years ago

#20849 closed enhancement (fixed)

Improve grs.py documentation

Reported by: David Lucas Owned by:
Priority: major Milestone: sage-7.6
Component: coding theory Keywords: rd3
Cc: Johan Rosenkilde 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:

Status badges

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 David Lucas

Summary: Improved grs.py documentationImprove grs.py documentation

comment:2 Changed 6 years ago by David Lucas

Branch: u/dlucas/rewrite_grs_docstrings

comment:3 Changed 6 years ago by David Lucas

Authors: David Lucas
Commit: 5dccaa08e04a65b3548bb802a6667479e975b0e3
Status: newneeds_review

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


New commits:

5dccaa0Improved documentation in grs.py

comment:4 Changed 6 years ago by Daniel Augot

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.
  • parity_column_multipliers the new docstring speaks about "positions scalars", while I guess it should be column multipliers, since the evaluation points are the same for the code and its dual.
  • weight_enumerator: it would be better to say "the polynomial whose coefficient of degree is [...]" this avoids naming the variable 'x'
  • (several places) the framework for describing the encoding could be made a bit more clear, using the vocables "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[...] over F` be the message
[...]
the encoding of `m` will be the following codeword:
Version 0, edited 6 years ago by Daniel Augot (next)

comment:5 Changed 6 years ago by ylchapuy

Status: needs_reviewneeds_work

needs rebase.

also:

  • typo: scalaers

comment:6 Changed 6 years ago by Clément Pernet

Branch: u/dlucas/rewrite_grs_docstringsu/cpernet/rewrite_grs_docstrings

comment:7 Changed 6 years ago by Clément Pernet

Commit: 5dccaa08e04a65b3548bb802a6667479e975b0e321ce2d0d52609fe68ec28a9bbb4a0be37db7e596

Rebased and fixed the scalaers typo. Other remarks by danielaugot still need to be adressed.


New commits:

21ce2d0Merge branch 'u/dlucas/rewrite_grs_docstrings' of git://trac.sagemath.org/sage into grs_docstring

comment:8 Changed 6 years ago by Clément Pernet

Keywords: rd3 added

comment:9 Changed 6 years ago by git

Commit: 21ce2d0d52609fe68ec28a9bbb4a0be37db7e596b2dfd4da6ed53f6168545c82ce1d694951eab51c

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

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

comment:10 Changed 6 years ago by Clément Pernet

Authors: David LucasDavid Lucas, Clément Pernet
Status: needs_workneeds_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 Daniel Augot

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 git

Commit: b2dfd4da6ed53f6168545c82ce1d694951eab51c20368066a5d873db5fbd3549648d846cc7bd5c7c

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

2036806fix typesetting of Hamming weight

comment:13 Changed 6 years ago by Clément Pernet

Typesetting updated.

comment:14 Changed 6 years ago by Daniel Augot

Reviewers: Daniel Augot
Status: needs_reviewpositive_review

comment:15 Changed 6 years ago by Volker Braun

Status: positive_reviewneeds_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 git

Commit: 20368066a5d873db5fbd3549648d846cc7bd5c7c1d9c6cbfe7b211b189aa11eb3f342a22f8067b59

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

1d9c6cbfix \text failing to compile in pdf.

comment:17 Changed 6 years ago by Clément Pernet

Status: needs_workneeds_review

Fixed. Ready for a (trivial) review.

comment:18 Changed 6 years ago by Daniel Augot

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 Daniel Augot

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 Daniel Augot

Status: needs_reviewpositive_review

comment:21 Changed 6 years ago by Travis Scrimshaw

Milestone: sage-7.3sage-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 Changed 6 years ago by Clément Pernet

Status: positive_reviewneeds_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 Clément Pernet

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

comment:24 in reply to:  22 Changed 6 years ago by Travis Scrimshaw

Branch: u/cpernet/rewrite_grs_docstringsu/tscrim/improve_grs-20849
Commit: 1d9c6cbfe7b211b189aa11eb3f342a22f8067b59e9fcb770f46c3521cb0b52ef43059a6cfe68a921
Reviewers: Daniel AugotDaniel 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 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:

e2d07fcMerge branch 'u/cpernet/rewrite_grs_docstrings' of git://trac.sagemath.org/sage into u/tscrim/improve_grs-20849
cde29f7Doing a little bit of extra cleanup.
e9fcb77Some details.

comment:25 Changed 6 years ago by Travis Scrimshaw

Status: needs_workneeds_review

comment:26 Changed 6 years ago by git

Commit: e9fcb770f46c3521cb0b52ef43059a6cfe68a921d631689019139a0f96784efd2555f1bd8d3fd930

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

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

comment:27 Changed 6 years ago by Travis Scrimshaw

ping

comment:28 Changed 6 years ago by Clément Pernet

Branch: u/tscrim/improve_grs-20849u/cpernet/improve_grs-20849

comment:29 Changed 6 years ago by Clément Pernet

Commit: d631689019139a0f96784efd2555f1bd8d3fd9302ae9428ae8e264beb34b031be9de72291af16dd8

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:

521fc25Use the more standard notation w for Hamming weight function [Roth'06].
2ae9428Merge branch 'u/tscrim/improve_grs-20849' of git://trac.sagemath.org/sage into u/tscrim/improve_grs-20849

comment:30 Changed 6 years ago by Clément Pernet

Reviewers: Daniel Augot, Travis ScrimshawDaniel Augot, Travis Scrimshaw, Clément Pernet

comment:31 Changed 6 years ago by Travis Scrimshaw

Status: needs_reviewpositive_review

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

comment:32 Changed 6 years ago by Volker Braun

Branch: u/cpernet/improve_grs-208492ae9428ae8e264beb34b031be9de72291af16dd8
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.