Opened 7 years ago
Closed 6 years ago
#20100 closed enhancement (fixed)
A new structure for Cyclic codes
Reported by:  David Lucas  Owned by:  

Priority:  major  Milestone:  sage7.5 
Component:  coding theory  Keywords:  sd75 
Cc:  Johan Rosenkilde, Clément Pernet  Merged in:  
Authors:  David Lucas, Julien Lavauzelle  Reviewers:  Daniel Augot, Yann LaigleChapuy 
Report Upstream:  N/A  Work issues:  
Branch:  d6f6bde (Commits, GitHub, GitLab)  Commit:  d6f6bdeb62e886638cb06f5ffa9ab752493779c3 
Dependencies:  #20284  Stopgaps: 
Description
This ticket proposes a new implementation for cyclic codes.
It contains:
 a new code class,
CyclicCode
,  a dedicated encoder to compute the generator matrix of a Cyclic Code, and
 an encoder whose message space is a polynomial ring.
This new structure has the following features:
 it is now possible to build a cyclic code in three different ways:
 either by using the generator polynomial, which is passed as an argument,
 either by using the defining set, which is passed as an argument (and can even be partial), or
 by passing a linear code as argument. In that case, a generator polynomial will be computed (if possible) and used to reformat the input code as a cyclic code.
 it also comes with a method to compute the BCH bound, either from a constructed code or directly from the necessary parameters.
Attachments (1)
Change History (74)
comment:1 Changed 7 years ago by
Branch:  → u/dlucas/cyclic_code 

comment:2 Changed 7 years ago by
Authors:  → David Lucas 

Commit:  → 6bcc1e724a6d0cbf249a2bb3fa4ef3f313100c9d 
Dependencies:  → #20039 
comment:3 Changed 7 years ago by
Commit:  6bcc1e724a6d0cbf249a2bb3fa4ef3f313100c9d → d9618bf3fe7b4958b5a8589ed08196a7c0468f7c 

Branch pushed to git repo; I updated commit sha1. New commits:
d9618bf  Fixed bug in defining_set

comment:4 Changed 7 years ago by
Status:  new → needs_review 

Ok, found it and fixed it. It's now open for review.
comment:5 Changed 7 years ago by
Commit:  d9618bf3fe7b4958b5a8589ed08196a7c0468f7c → bab6bf7098c5720a19f153ea7a6a91bea8a71834 

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
a074cb8  Changed my helper methods into private methods

6ad583f  Fixed bug in KeyEquation decoder's decode_to_code, added sanity checks, enhanced doctests

fca099e  Added new sanity check on the output of BW and Gao decoder

57dbfbf  Rewrote random_element method

7953d60  Proper sanity checks for output of decode_to_* methods

8673ac5  Optimized a bit polynomial division in Gao and BW

e1b6b09  Merged now postive reviewed #19666 and fixed conflicts

5093dce  Update to 7.1beta5

f3b4b11  Fixed typo in GuruswamiSudan doc

bab6bf7  Merge branch 'grs_decoders' into cyclic_code

comment:6 Changed 7 years ago by
I merged with latest beta, fixed conflicts with GRS branch, this is still open for review.
comment:7 Changed 7 years ago by
Commit:  bab6bf7098c5720a19f153ea7a6a91bea8a71834 → fbfcb9f2c59429050ddb23584d563594518baee5 

comment:8 Changed 7 years ago by
I fixed the broken doctests in code_constructions.py
.
They occured because the old implementation of CyclicCode
allowed to use generator polynomials which do not divide x^n  1
, n
being the length of the code.
I fixed this by replacing these polynomials (say, g
) by gcd(g, x^n  1)
.
This is still open for review.
comment:9 Changed 7 years ago by
Milestone:  sage7.1 → sage7.2 

comment:10 Changed 7 years ago by
Commit:  fbfcb9f2c59429050ddb23584d563594518baee5 → 0ee7bb69199de36738353c30ebb0104620502997 

comment:11 Changed 7 years ago by
I updated to the latest beta.
I also added a new optional argument: primitive_element
. If filled, it will be used as the "base root" for the consecutive root sequence of the generator polynomial over the splitting field.
comment:12 Changed 6 years ago by
Commit:  0ee7bb69199de36738353c30ebb0104620502997 → e459f107bb0b8c36149ec6a8ba9ef52c7e46aa06 

Branch pushed to git repo; I updated commit sha1. New commits:
97484cf  Merged into 7.3beta5, fixed conflicts, removed old field_embedding.py file

ea19a06  Removed subfield_subcode.py

61976d3  cyclic_code.py now uses relative_finite_field_extension.py

6335358  Fixed doctests in code_constructions.py

7366cbe  Enhanced module documentation, class' and global method's documentation

c328827  Removed __ne__ methods

141bb64  Enhanced documentation of various methods in CyclicCode

a210e73  Improved documentation of both encoders

e459f10  Helper methods are now private methods

comment:13 Changed 6 years ago by
Dependencies:  #20039 → #20284 

Milestone:  sage7.2 → sage7.3 
Hello,
I updated this ticket to latest beta version (7.3b5) and made some changes:
 I removed dependency to #20039.
 I improved documentation in many places, by defining terms more precisely.
 I removed now useless
__ne__
methods  I changed helper methods and made them private methods.
This is open for review.
David
comment:14 Changed 6 years ago by
Cc:  Johan Rosenkilde added 

comment:15 Changed 6 years ago by
Branch:  u/dlucas/cyclic_code → u/jsrn/cyclic_code 

comment:16 Changed 6 years ago by
Commit:  e459f107bb0b8c36149ec6a8ba9ef52c7e46aa06 → 922e688a2da5f855ed52bd8de4e0d55325e90179 

Fixed merge conflicts.
New commits:
922e688  Merge branch 'u/dlucas/cyclic_code' of git://trac.sagemath.org/sage into 20100_cyclic_codes

comment:17 Changed 6 years ago by
Commit:  922e688a2da5f855ed52bd8de4e0d55325e90179 → 7be49e607bc0000d4d60ad8f6ee23a24395192cb 

comment:18 Changed 6 years ago by
Branch:  u/jsrn/cyclic_code → u/jlavauzelle/cyclic_code 

comment:19 Changed 6 years ago by
Commit:  7be49e607bc0000d4d60ad8f6ee23a24395192cb → 5d2aef43433b313c026c20dfa518705eaf9fb2be 

Milestone:  sage7.3 → sage7.4 
Status:  needs_review → needs_work 
Hi,
As some sd75 participants may want to follow this ticket, I push some changes:
 I fixed some typos;
 I removed the probabilistic algorithm from the
find_generator_polynomial()
method, because it didn't always output the right result;  As we discussed quickly with Johan, only singleroot cyclic codes are now implemented (that is,
n
andq
must be coprimes);  I removed your local
_cyclotomic_cosets()
method;  I added some sanity checks;
 maybe other things
That's still a work in progress. Especially, it remarked that (non exhaustive):
__eq__
sometimes gives fake negatives (or type II errors). For example, when building cyclic codes from the same (mathematically speaking) generator polynomial, but belonging to two different instances ofPolynomialRing
. I don't remember why we let the user choose the primitive element (does the code really depend on that?)
 BCH bound computation will be hard to review :)
 the defining set looks to depend on the choice of
beta
, the nth root of unity.  I don't understand what is the
_known_generator_matrix
attribute  it seems weird that the
CyclicCodeVectorEncoder
encodes with a polynomial multiplication.
As I said, it's definitely a WIP.
Julien
New commits:
b961f67  Fixed typos. Replaced local `_cyclotomic_coset` function. Added some input checking.

5d2aef4  Fixed some bad newlines in strings.

comment:20 Changed 6 years ago by
Commit:  5d2aef43433b313c026c20dfa518705eaf9fb2be → 4c2e8c8263f4fe955083256c693bac7b2dba031c 

comment:21 Changed 6 years ago by
Commit:  4c2e8c8263f4fe955083256c693bac7b2dba031c → 84796b51bf88eba9c93a97e35b5d297249e37f63 

comment:22 Changed 6 years ago by
Hi,
I made some minor changes after merging the right 'develop' branch. I think now it compiles well (at least, in my machine; Joe Fields reported a compiling problem with guava in the previous push that I couldn't reproduce).
I reviewed almost all the code, i.e. all but the BCH bound. It looks quite fine now, but as it's a quite big patch, it clearly need a second review (besides, someone must review my own commits).
Julien
comment:23 Changed 6 years ago by
Authors:  David Lucas → David Lucas, Julien Lavauzelle 

Status:  needs_work → needs_review 
comment:24 Changed 6 years ago by
Commit:  84796b51bf88eba9c93a97e35b5d297249e37f63 → 5cff1dfdb1dd9f347a247e3577660c84b8b3e0f9 

Branch pushed to git repo; I updated commit sha1. New commits:
5cff1df  Fix a colon miss.

comment:25 Changed 6 years ago by
Hi David and Julien,
Using codes.CyclicCode.<TAB>
I went through all methods which are worth mentionning. I will just first make a few remarks about them and their associated docstrings.
codes.CyclicCode?
 "generator_pol" : in the sentence "the unique monic polynomial which divides every codeword" the meaning of "divides" is not mathematically clear. If you mean division in
F[X]/(X^n1)
, then you need to furthermore say it has lowest degree to make it unique.  "D": recall it is a set of (positive ? reduced mod n ?) integers
 "generator_pol" : in the sentence "the unique monic polynomial which divides every codeword" the meaning of "divides" is not mathematically clear. If you mean division in
codes.CyclicCode.bch_bound?
 is it possible to use
BCH_bound
to honor the authors ?
 is it possible to use
codes.CyclicCode.defining_set?
is very neat. For more completeness, I think it would be nice to briefly recall it depends on the choice of the primitive root (which is not shown or discussed in the examples).codes.CyclicCode.dual_code()
not a docstring problem, and more a design problem. As I just tested it, the resulting code is not a member of theCyclicCode class
.codes.CyclicCode.primitive_element?
in the string "Returns the primitive element that was used as a root the generator polynomial over the extension field.", the words "that was used" are not very clear, since it is not mentionned by which method it has been used. For instance it should be clear from the docstring what is the result when callingcodes.CyclicCode
using an arbritrary code. somehow the class inherits
C.variable_name
. Wouldn't it be a neat to use it to get the variable for the underlying polynomial ring ?
I will go through the implementation this afternoon.
Daniel
comment:26 Changed 6 years ago by
Branch:  u/jlavauzelle/cyclic_code → u/danielaugot/cyclic_code 

comment:27 Changed 6 years ago by
Commit:  5cff1dfdb1dd9f347a247e3577660c84b8b3e0f9 → c419639a5e12161c1cb790773645514a7ae2b926 

Proper test for the base ring to be finite field.
New commits:
c419639  in case (1) of the constructor, test if we have a field

comment:28 Changed 6 years ago by
Commit:  c419639a5e12161c1cb790773645514a7ae2b926 → 66a0d4782e69884be03ab79dae718d1dfaa5d611 

Branch pushed to git repo; I updated commit sha1. New commits:
66a0d47  test for a finite field in the (3) case of the constructor

comment:29 Changed 6 years ago by
Commit:  66a0d4782e69884be03ab79dae718d1dfaa5d611 → e8286a5ab4e6f0580e955f7e45f158ae0b652d61 

Branch pushed to git repo; I updated commit sha1. New commits:
e8286a5  proper error message

comment:30 Changed 6 years ago by
Commit:  e8286a5ab4e6f0580e955f7e45f158ae0b652d61 → 260e413df6b65b3fe97661e4ab5cfc4749708a59 

Branch pushed to git repo; I updated commit sha1. New commits:
260e413  proper error message II

comment:31 Changed 6 years ago by
About the internals and the implementation (I)
codes.CyclicCode
, case (3). Why do you check if primitive_element is nice ? Why do not you force it, since the user did not provide any such element in case (3)?
find_generator_polynomial
I would be happy to have a proof that the proposed algorithm is correct.
find_generator_polynomial
should return a monic polynomial. That would avoid doing the work in the constructor.
parity_check_matrix
is cached in the case of cyclic codes and not for general linear codes.
defining_set
: it would be nice to be able to give an alternate root as an argument, to see the defining set in another disguise.
 In many places, we have the following unpleasant code
s = 1 while not (q ** s  1) % n == 0: s += 1
to find the degree of the extension field where lies the nthroot of unity. Sage can compute the splitting field by itself, as follows
sage: K.<x>=PolynomialRing(GF(2)) sage: pol=x^311 sage: F=pol.splitting_field('y') sage: F Finite Field in y of size 2^5
I guess this could be used to have a more pleasant and mathematicall oriented coding style.
C'est tout pour ce soir.
Daniel
comment:32 Changed 6 years ago by
Commit:  260e413df6b65b3fe97661e4ab5cfc4749708a59 → 293f2a343d8b7d7373d94f0161bb8bf34ee5bfbd 

Branch pushed to git repo; I updated commit sha1. New commits:
293f2a3  replaced the HartmannTzeng reference by Roth

comment:33 Changed 6 years ago by
Branch:  u/danielaugot/cyclic_code → u/jlavauzelle/cyclic_code 

Changed 6 years ago by
Attachment:  proof_cyclic_code.pdf added 

A proof of the cyclic code criteria in the "find_generator_polynomial()" method.
comment:34 followup: 35 Changed 6 years ago by
Commit:  293f2a343d8b7d7373d94f0161bb8bf34ee5bfbd → 18f4629702ae81d08cc23d13ce382c5fcc1c12c5 

Hi,
I merged the latest beta (7.4.beta2). I also improved the doc according to Daniel's comments.
About your remarks:
 calling the method
BCH_bound
instead ofbch_bound
would not respect naming conventions;  I didn't made the
dual_code()
fix yet;  the problem with the variable name is not easy. A solution could be to internally force it to be
x
(however the code is built), but we don't want this to happen:sage: R = PolynomialRing(GF(2), 'y') sage: y = R.gen() sage: g = y + 1 sage: C = codesCyclicCode(g, 5) sage: C.generator_polynomial() == g False
 in the third construction (3), the user can provide a
primitive_element
;  I give a proof of my criteria in attachment;
 I agree for the optional alternate root as an argument of
defining_set
. Not done yet;  concerning the use of the
splitting_field()
method, I'm worrying about its computation cost, as we may only want the extension degree. But I agree your code is more pleasant to read.
Still a WIP (but reviews are welcome).
Julien
New commits:
45619f8  Merged latest beta (7.4.beta2).

ffa627b  Fixed double reference to [R06].

18f4629  Improved the documentation. Improved the way the generator polynomial is made monic.

comment:35 Changed 6 years ago by
Replying to jlavauzelle:
 concerning the use of the
splitting_field()
method, I'm worrying about its computation cost, as we may only want the extension degree. But I agree your code is more pleasant to read.
In all places where we computate s
, we immediately construct the splitting field as well, AFAICS. So we could indeed just use Daniel's loop (otherwise you are right, Julien, that computing the degree is much much faster. But in that case the loop should be wrapped as a function call).
Best, Johan
comment:36 Changed 6 years ago by
Commit:  18f4629702ae81d08cc23d13ce382c5fcc1c12c5 → 7c4ff2b6e739ae9acc00f70405af09d60cf11b9e 

comment:37 Changed 6 years ago by
Hi,
I fixed issues in defining_set()
and in primitive_element()
. I also completely changed bch_bound()
because it was not very clear and not that much efficient. I think now it's a bit faster.
Now it's really open for review.
Julien
comment:38 Changed 6 years ago by
Writing a BCHbound algorithm that is compact and fast yet readable is a fun challenge :) Here is my proposal, which is shorter than yours and to my mind more readable. I also added the small optimisation that the arithmetic step size doesn't need to be considered beyond n/2
.
On a related note, shouldn't bch_bound
always just output bch_parameters
?
def bch_bound(n, D, arithmetic=False): def longest_streak(step): max_len = 1 max_offset = 0 j = 0 while j < n: h = j while isD[h*step % n]: h += 1 if h  j > max_len: max_offset = j max_len = h  j j = h + 1 return (max_len, max_offset) isD = [ 1 if d in D else 0 for d in range(n) ] if not arithmetic: one_len, offset = longest_streak(1) return (one_len + 1, (1, offset)) else: step, (max_len, offset) = max([ (step, longest_streak(step)) for step in range(1,n//2) ], key=lambda (step,(step_len,_)): step_len) return (max_len + 1, (step, offset))
(This is not a review: I didn't look much at the rest of the code.)
comment:39 followup: 40 Changed 6 years ago by
For both implementation of the bch_bound
, I could be annoying and set D = range(n)
...
comment:40 followup: 42 Changed 6 years ago by
Replying to ylchapuy:
For both implementation of the
bch_bound
, I could be annoying and setD = range(n)
...
In which case, in O(n^2)
, the algorithm returns n+1 as minimum distance of the corresponding code. That input doesn't make much sense from a coding point of view, since it corresponds to the zero code of length n. Which according to some definitions has minimum distance n+1.
comment:41 Changed 6 years ago by
Hi
I am a bit concerned with the terminology primitive_element
where I think it should be primitive_root
or primitive_root_of_unity
.
In the finite field realm, primitive_element
really means a generator of the cyclic group of all invertible elements.
Even wikipedia says so https://en.wikipedia.org/wiki/Primitive_element_(finite_field).
On the other side, https://en.wikipedia.org/wiki/Root_of_unity#primitive really means what you want.
So primitive_element
here does not respect the standard finite field terminology.
Daniel
comment:42 followup: 44 Changed 6 years ago by
Replying to jsrn:
Replying to ylchapuy:
For both implementation of the
bch_bound
, I could be annoying and setD = range(n)
...In which case, in
O(n^2)
, the algorithm returns n+1 as minimum distance of the corresponding code. That input doesn't make much sense from a coding point of view, since it corresponds to the zero code of length n. Which according to some definitions has minimum distance n+1.
You may have to wait a bit more than O(n^2)
: while table_D[i*j % n] == 1: ...
and table_D
is all 1
From a coding point of view, I do agree this doesn't make sense, but is the user aware of that?
comment:43 Changed 6 years ago by
Hi Yann
thank you for looking at this ! And please be annoying : since our codes may be tortured with various operations for combining codes, it could well happen that someone ends with such a situation.
Concerning the cyclic code on length n
which are {0}
, I think n+1 is a correct math answer for the minimum distance d. At least it gives the MDS bound k+d=n+1.
So both Yann's remark and Johan's answer are correct.
Best,
Daniel
comment:44 Changed 6 years ago by
Replying to ylchapuy:
You may have to wait a bit more than
O(n^2)
:while table_D[i*j % n] == 1: ...
andtable_D
is all1
Hehe, yeah good point :)
comment:45 Changed 6 years ago by
Commit:  7c4ff2b6e739ae9acc00f70405af09d60cf11b9e → 238d19abb6d5516072ddd51c51fb50724c1b5fac 

comment:46 Changed 6 years ago by
Hi,
Thanks for all your comments! According to them, I made some changes:
 Johan, I picked your proposal (a way more readable, I didn't know it was possible to add optional parameters to the
max()
function, that's nice :)). I just replacedn//2
byn//2+1
in a loop.  I added some sanitychecks and fixed the problem raised by Yann (thanks!). As you said,
n+1
seems a good bound (at least) for the minimum distance of the zero code.  I replaced
primitive_element
byprimitive_root
.
Still open for review.
Julien
comment:47 Changed 6 years ago by
You may still come to an infinite loop if you skip the gcd
. Try e.g. bch_bound(6, [0,2,4], True)
. Also the key
trick is nice but useless, just reorder the tuple to (longest_streak(step), step)
and be happy with the lexicographic order.
I might read the rest of this if I find some more time.
comment:48 Changed 6 years ago by
Some remarks:
 arguments are silently ignored if you feed
__init__
too much (e.g. give a polynomial and a code)  line continuations are considered bad style in Python, prefer parentheses, braces, etc (see https://www.python.org/dev/peps/pep0008/#id19)
 use sorted instead of .sort() when possible
 why do your own caching and not using
@cached_method
(e.g. for check_polynomial)  instead of
self._known_generator_matrix
you could just checkself.generator_matrix.cache is not None
comment:49 Changed 6 years ago by
Commit:  238d19abb6d5516072ddd51c51fb50724c1b5fac → a4fb63ec915f9ccc3c6fce6bf720a82b5ca4d425 

Branch pushed to git repo; I updated commit sha1. New commits:
9790560  Fixed infinite loop.

932e940  Merged with latest beta (7.4.beta3) and fixed conflicts.

82e192d  Better argument cehcking in __init__.

2def8b1  Better line continuations.

0739aed  Use sorted() instead of .sort().

816b67d  Better use of cached methods.

a4fb63e  Fixed a small bug in defining_set().

comment:50 Changed 6 years ago by
Hi Yann,
Thanks again for you helpful comments (especially concerning the python/sage tricks). I pushed the corresponding fixes.
I also merged with the latest beta.
Julien
comment:51 Changed 6 years ago by
Cc:  Clément Pernet added 

So much for the 7.4beta3. develop does not compile, because linbox fails to compile on my fedora24. I can not test the ticket.
comment:52 Changed 6 years ago by
Hi
Line 425, there is this:
if primitive_root is not None: if (primitive_root not in Fsplit or multiplicative_order(primitive_root) != n): raise ValueError("primitive_root has to be an element of multiplicative order n in the extension field used to compute the generator polynomial")
where primitive_root
has been provided by the user in case (3) of the constructor. But Fsplit
has been constructed line 420:
while not (q ** s  1) % n == 0: s += 1 if s == 1: # splitting field is F Fsplit = F else: # must compute a splitting field Fsplit, F_to_Fsplit = F.extension(Integer(s), map = True)
this implies that the user may have provided his own, mathematically correct primitive_root
with his own finite field, which then will not be accepted by the code on line 425, because Fsplit
may be different from the field of user's primitive_root
(from sage point of view)
The following example triggers the problem
sage: F=GF(16,'a') sage: K=GF(256,'X') sage: alpha=K.primitive_element() sage: C=codes.CyclicCode(length = 255, field = F, D = [1,2], primitive_root=alpha)
and casts the error
ValueError: primitive_root has to be an element of multiplicative order n in the extension field used to compute the generator polynomial
It would be better to check first for the primitive_root
, and then build Fsplit
according to the one given by the user and build Fsplit
later if needed.
Daniel
comment:53 Changed 6 years ago by
Commit:  a4fb63ec915f9ccc3c6fce6bf720a82b5ca4d425 → 369e36c41171b098750d97067ae874159638e974 

comment:54 Changed 6 years ago by
Hi Daniel,
Thanks for the comment. You're completely right, I fixed the bug and merged with the latest beta.
Still open for review.
Julien
comment:55 Changed 6 years ago by
Commit:  369e36c41171b098750d97067ae874159638e974 → 176db5803d68b293dd76e74b04a0600beb64a3fb 

Branch pushed to git repo; I updated commit sha1. New commits:
176db58  Fixed doctests.

comment:57 Changed 6 years ago by
Hi,
 futurize imports (see e.g. #20945)
 you should avoid bare except (even if
RelativeFiniteFieldExtension(GF(5), GF(7))
fails in a strange way, which should probably be another ticket), at least check assertions withAssertionError
 you should add doctests for exceptions too
finally other more cosmetic remarks:
_complete_list
is always used after.coefficients(sparse = False)
, you could go one step further with( fordef _to_complete_list(poly, list_len): L = poly.coefficients(sparse = False) return L + [poly.base_ring().zero()] * (list_len  len(L))
parity_check_polynomial
tryh.reverse()
) regarding comment:31 , why not
Zmod(n)(q).multiplicative_order()
 use
root.log(alpha)
(special cased for each field) instead ofdiscrete_log
(generic)  here are some one liner you could use:
g = gcd(R(row.list()) for row in G)
if any(vector(c[k:] + c[:k]) not in code for k in range(n)):
H = matrix([l[i:] + l[:i] for i in range(nk)])
G = matrix([l[i:] + l[:i] for i in range(k)])
comment:58 Changed 6 years ago by
Commit:  176db5803d68b293dd76e74b04a0600beb64a3fb → 073af73cf1b5d2ee2bc89a71b833816dfabcb5cb 

comment:59 Changed 6 years ago by
Hi Yann,
Thanks again for the helpful comments. I did the modification.
Julien
comment:60 Changed 6 years ago by
Hi,
almost good, here are some more remarks:
 line 413, probably
ValueError as e
but what is the point of catching an exception to reraise it?
 found with flake8:
cyclic_code.py:60:1: F401 '.decoder.Decoder' imported but unused cyclic_code.py:66:1: F401 'sage.rings.polynomial.polynomial_ring.PolynomialRing_general' imported but unused cyclic_code.py:67:1: F401 'sage.rings.finite_rings.finite_field_constructor.GF' imported but unused cyclic_code.py:69:1: F401 'sage.categories.homset.Hom' imported but unused cyclic_code.py:70:1: F401 'sage.groups.generic.discrete_log' imported but unused cyclic_code.py:71:1: F401 'sage.misc.functional.multiplicative_order' imported but unused cyclic_code.py:103:5: F841 local variable 'x' is assigned to but never used cyclic_code.py:433:13: F841 local variable 'x' is assigned to but never used cyclic_code.py:725:9: F841 local variable 'R' is assigned to but never used
plus many PEP8 comments, mainly:
 no space around
=
for keyword arguments  whitespace after
[
or before]
comment:61 Changed 6 years ago by
Commit:  073af73cf1b5d2ee2bc89a71b833816dfabcb5cb → 0f4fe2758369ccdb0763047b4fb5c769bb53d2f5 

Branch pushed to git repo; I updated commit sha1. New commits:
0f4fe27  Improved code style.

comment:62 Changed 6 years ago by
Hi Yann,
I didn't know flake8, great tool. I improved the coding style according to it.
Julien
comment:63 Changed 6 years ago by
Commit:  0f4fe2758369ccdb0763047b4fb5c769bb53d2f5 → 6f235d690bfc58af7670d5218248e8283f58d900 

comment:64 Changed 6 years ago by
Keywords:  sd75 added 

Hi,
I updated to the latest beta (7.4.beta6).
Still open for review.
Julien
comment:65 Changed 6 years ago by
Commit:  6f235d690bfc58af7670d5218248e8283f58d900 → 8ee0d57a00d86f9945f75b1c497df907288e0ddd 

comment:66 Changed 6 years ago by
Milestone:  sage7.4 → sage7.5 

Reviewers:  → Daniel Augot, Yann LaigleChapuy 
Status:  needs_review → positive_review 
Previous comments have been taken into account, tests pass (failures on gentoo seem unrelated), coverage for the new file is 100%, doc builds and looks OK.
All good for me.
comment:67 Changed 6 years ago by
Status:  positive_review → needs_work 

Conflicts; Please wait for next beta and then fix the references:
[dochtml] [coding ] /mnt/disk/home/release/Sage/local/lib/python2.7/sitepackages/sage/coding/cyclic_code.py:docstring of sage.coding.cyclic_code:1: WARNING: citation not found: R06 [dochtml] [coding ] /mnt/disk/home/release/Sage/local/lib/python2.7/sitepackages/sage/coding/cyclic_code.py:docstring of sage.coding.cyclic_code:24: WARNING: citation not found: R06
comment:68 Changed 6 years ago by
Commit:  8ee0d57a00d86f9945f75b1c497df907288e0ddd → 75144a48e59efeaa4a022eaa4853e7880ba13825 

comment:69 Changed 6 years ago by
Status:  needs_work → needs_review 

comment:70 Changed 6 years ago by
Commit:  75144a48e59efeaa4a022eaa4853e7880ba13825 → d6f6bdeb62e886638cb06f5ffa9ab752493779c3 

comment:71 Changed 6 years ago by
Hi,
I updated to the latest beta (7.5.beta2) and fixed conflicts.
Following #21691, I also produced shorter printing for cyclic codes (basically, I removed the generator polynomial which can have a quite long printing).
Best,
Julien
comment:72 Changed 6 years ago by
Status:  needs_review → positive_review 

comment:73 Changed 6 years ago by
Branch:  u/jlavauzelle/cyclic_code → d6f6bdeb62e886638cb06f5ffa9ab752493779c3 

Resolution:  → fixed 
Status:  positive_review → closed 
I pushed my work, as it is almost done. There's still one bug to fix, documented in
defining_set
's broken docstring.Last 10 new commits:
Merged #19897
Updated introductory thematic tutorial with a paragraph on decoders and message spaces
Merge branch 'develop' into grs_decoders
Removed deprecated import
Updated to 7.1beta3 and fixed conflict
Removed __ne__ methods
Some fixes to the decoder. Merged with GRS decoders branch to have fast examples
Fixed a bug because of which it was impossible to build a PC matrix when the subfield was the prime field
Merge branch 'subfield_subcode' into cyclic_code
Class for cyclic and two encoders for this class