Sage: Ticket #20705: Classes for Reed Muller Codes
https://trac.sagemath.org/ticket/20705
<p>
This ticket proposes a implementation of Reed Muller Codes. It contains:
</p>
<blockquote>
<p>
two new code classes, QAryReedMullerCode and <a class="missing wiki">BinaryReedMullerCode?</a>, which implements the two classes of reed muller codes
two encoder classes, <a class="missing wiki">ReedMullerVectorEncoder?</a> and <a class="missing wiki">ReedMullerPolynomialEncoder?</a> which are used by both the code classes
some additional functions to assist in computations related to the polynomials.
</p>
</blockquote>
<p>
NOTE: Both the classes are implemented separately since they would have different decoders
</p>
<p>
I used the following code snippets to test them,
</p>
<pre class="wiki">#for q>2
code=ReedMullerCode(3, 2, 2)
print code.dimension()
E1=ReedMullerVectorEncoder(code)
E2=ReedMullerPolynomialEncoder(code)
R=PolynomialRing(code.base_field(),code.numberOfVariable,"x")
x0=R.gen(0)
x1=R.gen(1)
c1=E1.encode(vector(GF(3),[1,1,1,1,1,1]))
print c1
c2=E2.encode(1+x0+x1+x1^2+x1*x0)
print c2
D=LinearCodeSyndromeDecoder(code)
c=D.decode_to_code(vector(GF(3),[1, 2, 0, 0, 2, 0, 1, 1, 1]))
print c
print E2.unencode_nocheck(c)
print D.decode_to_message(vector(GF(3),[1,2,1,0,0,2,1,2,2]))
</pre><p>
The output of which was,
</p>
<pre class="wiki">6
(1, 0, 1, 0, 0, 2, 1, 2, 2)
(1, 2, 0, 0, 2, 1, 1, 1, 1)
(1, 2, 0, 0, 2, 1, 1, 1, 1)
x0*x1 + x1^2 + x0 + x1 + 1
(1, 1, 1, 1, 1, 1)
</pre><pre class="wiki">#for q=2
code=ReedMullerCode(2, 2, 4)
print code.dimension()
E1=ReedMullerVectorEncoder(code)
E2=ReedMullerPolynomialEncoder(code)
R=PolynomialRing(code.base_field(),code.numberOfVariable,"x")
x0=R.gen(0)
x1=R.gen(1)
x2=R.gen(2)
x3=R.gen(3)
c1=E1.encode(vector(GF(2),[1,1,1,1,1,0,0,0,1,0,0]))
print c1
c2=E2.encode(1+x0+x1+x2+x3*x2)
print c2
D=LinearCodeSyndromeDecoder(code)
c=D.decode_to_code(vector(GF(2),[1, 0, 0, 1, 0, 1, 1, 0, 1, 0, 0, 1, 1, 0, 0, 0]))
print c
print E2.unencode_nocheck(c)
print D.decode_to_message(vector(GF(2),[0, 0, 0, 1, 0, 1, 0, 1, 0, 1, 1, 0, 1, 0, 1, 0]))
</pre><p>
This gave the output as:
</p>
<pre class="wiki">11
(1, 0, 0, 1, 0, 1, 0, 1, 0, 1, 1, 0, 1, 0, 1, 0)
(1, 0, 0, 1, 0, 1, 1, 0, 1, 0, 0, 1, 1, 0, 0, 1)
(1, 0, 0, 1, 0, 1, 1, 0, 1, 0, 0, 1, 1, 0, 0, 1)
x2*x3 + x0 + x1 + x2 + 1
(1, 1, 1, 1, 1, 0, 0, 0, 1, 0, 0)
</pre>en-usSagehttps://trac.sagemath.org/chrome/site/logo_sagemath_trac.png
https://trac.sagemath.org/ticket/20705
Trac 1.1.6panda314Sun, 29 May 2016 16:07:27 GMTbranch set
https://trac.sagemath.org/ticket/20705#comment:1
https://trac.sagemath.org/ticket/20705#comment:1
<ul>
<li><strong>branch</strong>
set to <em>u/panda314/classes_for_reed_muller_codes</em>
</li>
</ul>
Ticketpanda314Sun, 29 May 2016 16:22:19 GMTdescription changed; commit set
https://trac.sagemath.org/ticket/20705#comment:2
https://trac.sagemath.org/ticket/20705#comment:2
<ul>
<li><strong>commit</strong>
set to <em>9fb9b697ceb1e21ee450809a3447695d46f075bb</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/20705?action=diff&version=2">diff</a>)
</li>
</ul>
<p>
Replying to <a class="closed ticket" href="https://trac.sagemath.org/ticket/20705" title="enhancement: Classes for Reed Muller Codes (closed: fixed)">panda314</a>:
</p>
<blockquote class="citation">
<p>
This ticket proposes a implementation of Reed Muller Codes. It contains:
</p>
<blockquote>
<p>
two new code classes, QAryReedMullerCode and <a class="missing wiki">BinaryReedMullerCode?</a>, which implements the two classes of reed muller codes
two encoder classes, <a class="missing wiki">ReedMullerVectorEncoder?</a> and <a class="missing wiki">ReedMullerPolynomialEncoder?</a> which are used by both the code classes
some additional functions to assist in computations related to the polynomials.
</p>
</blockquote>
<p>
NOTE: Both the classes are implemented separately since they would have different decoders
</p>
</blockquote>
<hr />
<p>
New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="http://git.sagemath.org/sage.git/commit/?id=0ffd780234a19321ad2cee7676ebbef26bdc8bdf"><span class="icon"></span>0ffd780</a></td><td><code>adding ReedMullerCode.py containing support for encoding of Reed Muller Codes</code>
</td></tr><tr><td><a class="ext-link" href="http://git.sagemath.org/sage.git/commit/?id=9fb9b697ceb1e21ee450809a3447695d46f075bb"><span class="icon"></span>9fb9b69</a></td><td><code>Merge branch 'RMCode' into t/20705/classes_for_reed_muller_codes</code>
</td></tr></table>
TicketgitMon, 30 May 2016 12:06:46 GMTcommit changed
https://trac.sagemath.org/ticket/20705#comment:3
https://trac.sagemath.org/ticket/20705#comment:3
<ul>
<li><strong>commit</strong>
changed from <em>9fb9b697ceb1e21ee450809a3447695d46f075bb</em> to <em>ba4de76e487f27cd572856735c9b3ff960f25664</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="http://git.sagemath.org/sage.git/commit/?id=ba4de76e487f27cd572856735c9b3ff960f25664"><span class="icon"></span>ba4de76</a></td><td><code>adding documentation</code>
</td></tr></table>
TicketgitMon, 30 May 2016 12:40:46 GMTcommit changed
https://trac.sagemath.org/ticket/20705#comment:4
https://trac.sagemath.org/ticket/20705#comment:4
<ul>
<li><strong>commit</strong>
changed from <em>ba4de76e487f27cd572856735c9b3ff960f25664</em> to <em>262e32068c96371b3cc28b4d0c16f2eddecb661d</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="http://git.sagemath.org/sage.git/commit/?id=262e32068c96371b3cc28b4d0c16f2eddecb661d"><span class="icon"></span>262e320</a></td><td><code>adding documentation for additional functions</code>
</td></tr></table>
TicketdlucasTue, 31 May 2016 12:24:59 GMTcc set
https://trac.sagemath.org/ticket/20705#comment:5
https://trac.sagemath.org/ticket/20705#comment:5
<ul>
<li><strong>cc</strong>
<em>dlucas</em> added
</li>
</ul>
TicketgitWed, 01 Jun 2016 11:53:20 GMTcommit changed
https://trac.sagemath.org/ticket/20705#comment:6
https://trac.sagemath.org/ticket/20705#comment:6
<ul>
<li><strong>commit</strong>
changed from <em>262e32068c96371b3cc28b4d0c16f2eddecb661d</em> to <em>c5af6d27cb948eb1d41419622a0640b9fdf16377</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="http://git.sagemath.org/sage.git/commit/?id=3fc4924e3f8d66c2595b083c196a5f4209f7528d"><span class="icon"></span>3fc4924</a></td><td><code>minor edits</code>
</td></tr><tr><td><a class="ext-link" href="http://git.sagemath.org/sage.git/commit/?id=c5af6d27cb948eb1d41419622a0640b9fdf16377"><span class="icon"></span>c5af6d2</a></td><td><code>Some adjustments to way functions take their inputs and adding the option of giving your own polynomial ring to ReedMullerPolynomialEncoder</code>
</td></tr></table>
TicketgitWed, 01 Jun 2016 13:23:17 GMTcommit changed
https://trac.sagemath.org/ticket/20705#comment:7
https://trac.sagemath.org/ticket/20705#comment:7
<ul>
<li><strong>commit</strong>
changed from <em>c5af6d27cb948eb1d41419622a0640b9fdf16377</em> to <em>ca93a4b3ce5ab53febff2cf554bab057ce61ac0f</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="http://git.sagemath.org/sage.git/commit/?id=ca93a4b3ce5ab53febff2cf554bab057ce61ac0f"><span class="icon"></span>ca93a4b</a></td><td><code>removing ReedMullerCode() of guava.py from the implementation</code>
</td></tr></table>
TicketgitWed, 01 Jun 2016 17:44:39 GMTcommit changed
https://trac.sagemath.org/ticket/20705#comment:8
https://trac.sagemath.org/ticket/20705#comment:8
<ul>
<li><strong>commit</strong>
changed from <em>ca93a4b3ce5ab53febff2cf554bab057ce61ac0f</em> to <em>23ff289a86e2d4266512e631f50ee75c6338ccd8</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="http://git.sagemath.org/sage.git/commit/?id=23ff289a86e2d4266512e631f50ee75c6338ccd8"><span class="icon"></span>23ff289</a></td><td><code>debugged and ran the doctests</code>
</td></tr></table>
TicketdlucasThu, 02 Jun 2016 09:35:58 GMTstatus changed
https://trac.sagemath.org/ticket/20705#comment:9
https://trac.sagemath.org/ticket/20705#comment:9
<ul>
<li><strong>status</strong>
changed from <em>new</em> to <em>needs_review</em>
</li>
</ul>
<p>
Hello,
</p>
<p>
I started reading your code.
Before giving actual comments on the code itself, here are some general comments:
</p>
<ul><li>there is certain conventions to respect while writing Python code. You can find a summary of these conventions <a class="ext-link" href="http://doc.sagemath.org/html/en/developer/coding_basics.html#python-code-style"><span class="icon"></span>here</a>.
</li></ul><ul><li>lines are supposed to be less than 80 characters long. As I think it's an old rule, I'm completely ok if some lines are around 100 characters, especially in the doctests block which might be hard to break. However, some lines in your code are just too long: e.g. l. 58 is 161 characters long, l.92 is 144 characters long...). A general tip: in Python, as long as you are in the same parenthesis block, you can jump to the next line without any special character. For instance, this:
</li></ul><pre class="wiki">poly=poly+z*multivariatePolynomialInterpolation([evaluation2[i][k] for i in range(nq)], numberOfVariable-1, order-k, q, finiteField, _R)
</pre><p>
can be written like this:
</p>
<pre class="wiki">poly=poly+z*multivariatePolynomialInterpolation([evaluation2[i][k] for i in range(nq)],
numberOfVariable-1, order-k, q, finiteField, _R)
</pre><ul><li>on names: in python, methods and variables names are written like this <code>function_name</code> and <code>variable_name</code> (and not <code>functionName</code> and <code>variableName</code>). Module names should follow the same convention (<code>reed_muller_code.py</code> instead of <code>ReedMullerCode.py</code>).
</li></ul><ul><li>You wrote the documentation for a few methods like this:
</li></ul><pre class="wiki">r"""
DOC
"""
def my_func:
code
</pre><p>
but it should *always* be like this:
</p>
<pre class="wiki">def my_func:
r"""
DOC
"""
code
</pre><ul><li>More of the same, this DOC block should always be formatted like this:
</li></ul><pre class="wiki">def my_func(i1, i2):
r"""
Short description of my_func return value
(Should be something like "Returns ....")
[optional] more details
INPUT:
- ``i1`` -- description of i1
- ``i2`` -- description of i2
[optional] OUTPUT
EXAMPLES::
sage: ...
[optional] TESTS::
sage: ...
</pre><ul><li>you should always put one whitespace before and after operators (exception: precedence), e.g:
<ul><li><code>a = b + c</code>
</li><li><code>a = (b+c) * (b-c)</code>
</li></ul></li></ul><ul><li>you used full names for variables everywhere but for <code>_R</code>. I think it would be better to rename it <code>polynomial_ring</code> or something like that.
</li></ul><p>
I'll run tests this afternoon and probably come back with more comments on the code itself.
Don't worry about all my comments in formatting, it's your first big ticket for Sage, so it's perfectly normal to use wrong coding conventions. It takes some time to get used to it <code>;)</code>
</p>
<p>
Best,
</p>
<p>
David
</p>
TicketdlucasThu, 02 Jun 2016 13:02:16 GMTstatus changed
https://trac.sagemath.org/ticket/20705#comment:10
https://trac.sagemath.org/ticket/20705#comment:10
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
Disclaimer: the following remarks are only related to the code in itsef. I did not run (yet) extensive tests on border cases and larger cases than the ones covered by doctests.
</p>
<p>
General remark on exceptions: don't need to write "Incorrect data-type ..." every time, as <code>ValueError</code> will be printed on the screen and it means there is something wrong with the input.
</p>
<p>
Another remark on documentation: you have to add this module to <code>src/doc/en/reference/coding/index.rst</code> to make it appear in the online documentation. For now, this module is never considered when compiling documentation - which means, even if you run <code>sage --docbuild reference/coding</code>, you won't be able to know if there are some bugs in your doc.
</p>
<p>
<em>binomial_sum</em>
</p>
<p>
I think it might be better to make it a private method (which can be done by calling it _binomial_sum).
</p>
<p>
You could have used already implemented methods, ending up with something like:
</p>
<p>
<code>return sum(binomial(n, i) for i in range(k+1))</code> or <code>for i in range(k+1): s += binomial(n, i)</code>
</p>
<p>
BUT it's much slower than you implementation (up to a factor of 10 according to <code>timeit</code>). So, we should just keep what you did :)
</p>
<p>
<em>multivariate_polynomial_interpolation</em>
</p>
<p>
I'd make it a private method as it's only used internally as a helper.
I would also change the name <code>_R</code> to <code>polynomial_ring</code>. It's probably something
you copied from my poor naming choice in grs.py... I changed it in <a class="closed ticket" href="https://trac.sagemath.org/ticket/20744" title="enhancement: Polynomial encoder for GRS codes fails if variable name is not x (closed: fixed)">#20744</a> in the meanwhile ;)
</p>
<p>
I'd also rename some variables: <code>evaluation2</code> might be <code>multipoint_evaluation_list</code> or something like that. I would also change the name <code>nq</code>, but I don't have any idea here. I would also rename <code>finiteField</code> as <code>base_field</code> for consistency.
You should be careful with <code>0</code>s and <code>1</code>s: the <code>0</code> l. 87 is not the same as the one on line 89: the former is the zero of the base field while the latter is the zero of the multivariate polynomial ring where <code>poly</code> lives.
I recommend using specific variables for these zeroes/ones, which you can define as follows: <code>base_field_zero = base_field.zero()</code>.
</p>
<p>
<em><a class="missing wiki">ReedMullerCode?</a></em>
</p>
<ul><li>you need to add <code>::</code> in the end of your second example's descriptive sentence.
</li></ul><p>
<em>General remark on classes</em>
</p>
<ul><li>Most of your class parameters are public variables that one has to call directly (e.g. <code>C.numberOfVariable</code>) to access. I would change this: make all these variables private (prefix them by an underscore) and write getter methods.
</li></ul><p>
One should call <code>C.number_of_variables()</code> to the number of variables used.
</p>
<p>
<em>QAry/BinaryReedMullerCode</em>
</p>
<ul><li>You could add a method to compute the minimum distance, as we know a closed formula to compute it, it's quite a shame to call the exhaustive search algortihm if one types <code>My_rm_code.minimum_distance()</code>.
</li></ul><p>
If <code>q</code> is 2, <code>D = 2^(m-d)</code>, otherwise it's <code>q^m - d*q^(m-1)</code>, with <code>m</code> the number of variables and <code>d</code> the order.
<em>
QAryReedMullerCode</em>
</p>
<ul><li>I would add a <code>WARNING::</code> block which states that the order has to be less than <code>q</code> to be sure users know about it. Of course, it will be removed later on, but the more informative the better.
</li></ul><p>
<em>Vectorial encoder</em>
</p>
<ul><li>I noticed you compute the generator matrix at construction time. I strongly disagree with this behaviour: as the generator matrix computation can be heavy, I advise you to only compute it when the needed (i.e. in the method <code>generator_matrix</code>). The method <code>generator_matrix</code> should also be cached (use the decorator <code>@cached_method</code>).
</li></ul><ul><li>On the generator matrix computation. I think you should use iterators when possible. Especially, the lines
</li></ul><pre class="wiki">baseFieldTuple=Tuples(baseField.list(),numberOfVariable)
[...] for x in baseFieldTuple.list()
</pre><p>
will kill you. For example, over <code>GF(31)</code>, with only 4 variables, it takes more than 3 minutes to compute the list of all tuples on my laptop (16 GB RAM).
Using an iterator will not compute the full list of tuples, which saves time and memory for larger codes.
</p>
<p>
Same for <code>exponents</code>: over GF(23), 4 variables and dimension 6, it takes circa 30 seconds to execute this on my laptop:
</p>
<pre class="wiki">exponents=Subsets(range(numberOfVariable)*(q-1), submultiset=True)[0:code.dimension()]
</pre><p>
Best,
</p>
<p>
David
</p>
Ticketpanda314Thu, 02 Jun 2016 13:30:55 GMT
https://trac.sagemath.org/ticket/20705#comment:11
https://trac.sagemath.org/ticket/20705#comment:11
<p>
Thanks. I have started implementing some of the changes you have suggested.
Regarding formatting, is there any format code or something that i can plug in a ide interface and automatically implement them? It will be tedious to go through the code and manually do them.
</p>
<p>
Regarding private functions. I had not added examples for them because doctest would not be able to execute them since i had not listed them anywhere. Will writing them as _<private functions> solve that?
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/20705#comment:10" title="Comment 10">dlucas</a>:
</p>
<blockquote class="citation">
<p>
Disclaimer: the following remarks are only related to the code in itsef. I did not run (yet) extensive tests on border cases and larger cases than the ones covered by doctests.
</p>
<p>
General remark on exceptions: don't need to write "Incorrect data-type ..." every time, as <code>ValueError</code> will be printed on the screen and it means there is something wrong with the input.
</p>
<p>
Another remark on documentation: you have to add this module to <code>src/doc/en/reference/coding/index.rst</code> to make it appear in the online documentation. For now, this module is never considered when compiling documentation - which means, even if you run <code>sage --docbuild reference/coding</code>, you won't be able to know if there are some bugs in your doc.
</p>
<p>
<em>binomial_sum</em>
</p>
<p>
I think it might be better to make it a private method (which can be done by calling it _binomial_sum).
</p>
<p>
You could have used already implemented methods, ending up with something like:
</p>
<p>
<code>return sum(binomial(n, i) for i in range(k+1))</code> or <code>for i in range(k+1): s += binomial(n, i)</code>
</p>
<p>
BUT it's much slower than you implementation (up to a factor of 10 according to <code>timeit</code>). So, we should just keep what you did :)
</p>
<p>
<em>multivariate_polynomial_interpolation</em>
</p>
<p>
I'd make it a private method as it's only used internally as a helper.
I would also change the name <code>_R</code> to <code>polynomial_ring</code>. It's probably something
you copied from my poor naming choice in grs.py... I changed it in <a class="closed ticket" href="https://trac.sagemath.org/ticket/20744" title="enhancement: Polynomial encoder for GRS codes fails if variable name is not x (closed: fixed)">#20744</a> in the meanwhile ;)
</p>
<p>
I'd also rename some variables: <code>evaluation2</code> might be <code>multipoint_evaluation_list</code> or something like that. I would also change the name <code>nq</code>, but I don't have any idea here. I would also rename <code>finiteField</code> as <code>base_field</code> for consistency.
You should be careful with <code>0</code>s and <code>1</code>s: the <code>0</code> l. 87 is not the same as the one on line 89: the former is the zero of the base field while the latter is the zero of the multivariate polynomial ring where <code>poly</code> lives.
I recommend using specific variables for these zeroes/ones, which you can define as follows: <code>base_field_zero = base_field.zero()</code>.
</p>
<p>
<em><a class="missing wiki">ReedMullerCode?</a></em>
</p>
<ul><li>you need to add <code>::</code> in the end of your second example's descriptive sentence.
</li></ul><p>
<em>General remark on classes</em>
</p>
<ul><li>Most of your class parameters are public variables that one has to call directly (e.g. <code>C.numberOfVariable</code>) to access. I would change this: make all these variables private (prefix them by an underscore) and write getter methods.
</li></ul><p>
One should call <code>C.number_of_variables()</code> to the number of variables used.
</p>
<p>
<em>QAry/BinaryReedMullerCode</em>
</p>
<ul><li>You could add a method to compute the minimum distance, as we know a closed formula to compute it, it's quite a shame to call the exhaustive search algortihm if one types <code>My_rm_code.minimum_distance()</code>.
</li></ul><p>
If <code>q</code> is 2, <code>D = 2^(m-d)</code>, otherwise it's <code>q^m - d*q^(m-1)</code>, with <code>m</code> the number of variables and <code>d</code> the order.
<em>
QAryReedMullerCode</em>
</p>
<ul><li>I would add a <code>WARNING::</code> block which states that the order has to be less than <code>q</code> to be sure users know about it. Of course, it will be removed later on, but the more informative the better.
</li></ul><p>
<em>Vectorial encoder</em>
</p>
<ul><li>I noticed you compute the generator matrix at construction time. I strongly disagree with this behaviour: as the generator matrix computation can be heavy, I advise you to only compute it when the needed (i.e. in the method <code>generator_matrix</code>). The method <code>generator_matrix</code> should also be cached (use the decorator <code>@cached_method</code>).
</li></ul><ul><li>On the generator matrix computation. I think you should use iterators when possible. Especially, the lines
</li></ul><pre class="wiki">baseFieldTuple=Tuples(baseField.list(),numberOfVariable)
[...] for x in baseFieldTuple.list()
</pre><p>
will kill you. For example, over <code>GF(31)</code>, with only 4 variables, it takes more than 3 minutes to compute the list of all tuples on my laptop (16 GB RAM).
Using an iterator will not compute the full list of tuples, which saves time and memory for larger codes.
</p>
<p>
Same for <code>exponents</code>: over GF(23), 4 variables and dimension 6, it takes circa 30 seconds to execute this on my laptop:
</p>
<pre class="wiki">exponents=Subsets(range(numberOfVariable)*(q-1), submultiset=True)[0:code.dimension()]
</pre><p>
Best,
</p>
<p>
David
</p>
</blockquote>
TicketdlucasThu, 02 Jun 2016 13:44:56 GMT
https://trac.sagemath.org/ticket/20705#comment:12
https://trac.sagemath.org/ticket/20705#comment:12
<blockquote class="citation">
<p>
Thanks. I have started implementing some of the changes you have suggested.
</p>
</blockquote>
<p>
Cool!
</p>
<blockquote class="citation">
<p>
Regarding formatting, is there any format code or something that i can plug in a ide interface and automatically implement them? It will be tedious to go through the code and manually do them.
</p>
</blockquote>
<p>
Not that I know, I'm afraid. I'm not using any IDE but vim, so I can't be sure though.
But if it's just to change <code>MyVariable</code> to <code>my_variable</code>, any text editor has a search and replace feature which will do the job. Of course, you will have to feed it with the target name everytime...
</p>
<blockquote class="citation">
<p>
Regarding private functions. I had not added examples for them because doctest would not be able to execute them since i had not listed them anywhere. Will writing them as _<private functions> solve that?
</p>
</blockquote>
<p>
Actually, doctests will be executed. Making these methods private with <code>_name</code> will make them disappear from the online doc, but the doctesting framework will run them as well. See <code>_punctured_form</code> in <code>grs.py</code> for an example.
</p>
<p>
Best,
</p>
<p>
David
</p>
Ticketpanda314Thu, 02 Jun 2016 15:51:45 GMT
https://trac.sagemath.org/ticket/20705#comment:13
https://trac.sagemath.org/ticket/20705#comment:13
<p>
Hey, so correct me if i am wrong, [x for x in base_field_tuple] is implicitly implemented using iterator. If that's so i think the following implementation should get rid all the redundant iterations through the sets.
</p>
<p>
exponent=exponents.first()
for i in range(dimension):
</p>
<blockquote>
<p>
matrix_list.append([reduce(mul, [x[i] for i in exponent],1) for x in base_field_tuple])
exponent=exponents.next(exponent)
</p>
</blockquote>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/20705#comment:10" title="Comment 10">dlucas</a>:
</p>
<blockquote class="citation">
<p>
Disclaimer: the following remarks are only related to the code in itsef. I did not run (yet) extensive tests on border cases and larger cases than the ones covered by doctests.
</p>
<p>
General remark on exceptions: don't need to write "Incorrect data-type ..." every time, as <code>ValueError</code> will be printed on the screen and it means there is something wrong with the input.
</p>
<p>
Another remark on documentation: you have to add this module to <code>src/doc/en/reference/coding/index.rst</code> to make it appear in the online documentation. For now, this module is never considered when compiling documentation - which means, even if you run <code>sage --docbuild reference/coding</code>, you won't be able to know if there are some bugs in your doc.
</p>
<p>
<em>binomial_sum</em>
</p>
<p>
I think it might be better to make it a private method (which can be done by calling it _binomial_sum).
</p>
<p>
You could have used already implemented methods, ending up with something like:
</p>
<p>
<code>return sum(binomial(n, i) for i in range(k+1))</code> or <code>for i in range(k+1): s += binomial(n, i)</code>
</p>
<p>
BUT it's much slower than you implementation (up to a factor of 10 according to <code>timeit</code>). So, we should just keep what you did :)
</p>
<p>
<em>multivariate_polynomial_interpolation</em>
</p>
<p>
I'd make it a private method as it's only used internally as a helper.
I would also change the name <code>_R</code> to <code>polynomial_ring</code>. It's probably something
you copied from my poor naming choice in grs.py... I changed it in <a class="closed ticket" href="https://trac.sagemath.org/ticket/20744" title="enhancement: Polynomial encoder for GRS codes fails if variable name is not x (closed: fixed)">#20744</a> in the meanwhile ;)
</p>
<p>
I'd also rename some variables: <code>evaluation2</code> might be <code>multipoint_evaluation_list</code> or something like that. I would also change the name <code>nq</code>, but I don't have any idea here. I would also rename <code>finiteField</code> as <code>base_field</code> for consistency.
You should be careful with <code>0</code>s and <code>1</code>s: the <code>0</code> l. 87 is not the same as the one on line 89: the former is the zero of the base field while the latter is the zero of the multivariate polynomial ring where <code>poly</code> lives.
I recommend using specific variables for these zeroes/ones, which you can define as follows: <code>base_field_zero = base_field.zero()</code>.
</p>
<p>
<em><a class="missing wiki">ReedMullerCode?</a></em>
</p>
<ul><li>you need to add <code>::</code> in the end of your second example's descriptive sentence.
</li></ul><p>
<em>General remark on classes</em>
</p>
<ul><li>Most of your class parameters are public variables that one has to call directly (e.g. <code>C.numberOfVariable</code>) to access. I would change this: make all these variables private (prefix them by an underscore) and write getter methods.
</li></ul><p>
One should call <code>C.number_of_variables()</code> to the number of variables used.
</p>
<p>
<em>QAry/BinaryReedMullerCode</em>
</p>
<ul><li>You could add a method to compute the minimum distance, as we know a closed formula to compute it, it's quite a shame to call the exhaustive search algortihm if one types <code>My_rm_code.minimum_distance()</code>.
</li></ul><p>
If <code>q</code> is 2, <code>D = 2^(m-d)</code>, otherwise it's <code>q^m - d*q^(m-1)</code>, with <code>m</code> the number of variables and <code>d</code> the order.
<em>
QAryReedMullerCode</em>
</p>
<ul><li>I would add a <code>WARNING::</code> block which states that the order has to be less than <code>q</code> to be sure users know about it. Of course, it will be removed later on, but the more informative the better.
</li></ul><p>
<em>Vectorial encoder</em>
</p>
<ul><li>I noticed you compute the generator matrix at construction time. I strongly disagree with this behaviour: as the generator matrix computation can be heavy, I advise you to only compute it when the needed (i.e. in the method <code>generator_matrix</code>). The method <code>generator_matrix</code> should also be cached (use the decorator <code>@cached_method</code>).
</li></ul><ul><li>On the generator matrix computation. I think you should use iterators when possible. Especially, the lines
</li></ul><pre class="wiki">baseFieldTuple=Tuples(baseField.list(),numberOfVariable)
[... ...] for x in baseFieldTuple.list()
</pre><p>
will kill you. For example, over <code>GF(31)</code>, with only 4 variables, it takes more than 3 minutes to compute the list of all tuples on my laptop (16 GB RAM).
Using an iterator will not compute the full list of tuples, which saves time and memory for larger codes.
</p>
<p>
Same for <code>exponents</code>: over GF(23), 4 variables and dimension 6, it takes circa 30 seconds to execute this on my laptop:
</p>
<pre class="wiki">exponents=Subsets(range(numberOfVariable)*(q-1), submultiset=True)[0:code.dimension()]
</pre><p>
Best,
</p>
<p>
David
</p>
</blockquote>
TicketgitThu, 02 Jun 2016 17:58:45 GMTcommit changed
https://trac.sagemath.org/ticket/20705#comment:14
https://trac.sagemath.org/ticket/20705#comment:14
<ul>
<li><strong>commit</strong>
changed from <em>23ff289a86e2d4266512e631f50ee75c6338ccd8</em> to <em>5129c915c146098f2c9c03ce89ca992b32e86609</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="http://git.sagemath.org/sage.git/commit/?id=5129c915c146098f2c9c03ce89ca992b32e86609"><span class="icon"></span>5129c91</a></td><td><code>reformatting the file according to standards and rewriting the code to optimise computation time</code>
</td></tr></table>
TicketdlucasFri, 03 Jun 2016 08:20:16 GMT
https://trac.sagemath.org/ticket/20705#comment:15
https://trac.sagemath.org/ticket/20705#comment:15
<blockquote class="citation">
<p>
Hey, so correct me if i am wrong, <code>[x for x in base_field_tuple]</code> is implicitly implemented using iterator. If that's so i think the following implementation should get rid all the redundant iterations through the sets.
</p>
</blockquote>
<p>
Yes, absolutely.
</p>
<p>
I'm reading your modified code, I'll come back with more comments later on.
</p>
TicketdlucasFri, 03 Jun 2016 13:02:37 GMT
https://trac.sagemath.org/ticket/20705#comment:16
https://trac.sagemath.org/ticket/20705#comment:16
<p>
Hello,
</p>
<p>
Some more comments:
</p>
<ul><li>I noticed you add both <code>ReedMullerCode</code>, <code>BinaryReedMullerCode</code> and <code>QAryReedMullerCode</code> to <code>codes_catalog.py</code>. But with our design, only <code>ReedMullerCode</code> should be in this file, as the user should never access the classes themselves.
</li></ul><h2 id="_multivariate_polynomial_interpolation"><code>_multivariate_polynomial_interpolation</code></h2>
<ul><li>You left a reference to <code>_R</code> in the docstring while it no longer exists
</li><li>Is the base field (and the cardinality) really needed? I would rather remove them from the list of inputs and recover them from <code>evaluation</code>. This way, one has to provide <code>evaluation</code> as a vector over a finite field, you get this finite field by doing <code>F = evaluation.base_ring()</code>, you check that <code>F</code> is the base ring of <code>polynomial_ring</code> and that's it. You can get <code>q</code> with the appropriate call on <code>F</code>.
</li><li>As my previous comment implies, I would only allow <code>evaluation</code> to be a vector (and not a list). After all, it's a private method you only use to recover the original message as a polynomial from a codeword, and the latter is necessarily a vector, isn't it?
</li><li>l. 105: is there a way to avoid a call to <code>base_field.list()</code>? I know that in most cases, this list will be rather small, but it's not really efficient to build a full list (especially in a recursive pattern, because you will build it a lot of times...). This time again, I'd suggest an iterator over the finite field.
</li></ul><h2 id="ReedMullerCode"><code>ReedMullerCode</code></h2>
<ul><li>the <code>WARNING</code> block should be added in the module documentation as well. The syntax on this block is the following
<pre class="wiki">.. WARNING::
body of the block
</pre>See <code>linear_code.py</code>, line 682 for an example.
I would also change the message. I propose <code>For now, this implementation only supports Reed-Muller codes whose order is less than q</code>. Such a message implies it will be extended later, I think it's more positive this way ;)
</li></ul><h2 id="Q-aryandBinaryclasses">Q-ary and Binary classes</h2>
<ul><li>I think it's not necessary to save <code>q</code> as a class parameter. The user can access it by typing <code>C.base_field().cardinality()</code>, it's not something specific to the class (or something hard to "compute"), so it should not be saved.
</li></ul><ul><li>I prefer full names for user-accessed methods. I'm okay with <code>num_var</code> as an internal variable, but its getter should be called <code>number_of_variables</code> instead.
</li></ul><h2 id="Vectorialencoder">Vectorial encoder</h2>
<ul><li>In <code>generator_matrix</code>, you're calling <code>self.code()</code> a lot of times. I think you should store it in a local variable which will reduce the amount of calls to this method.
</li></ul><p>
Best,
</p>
<p>
David
</p>
Ticketpanda314Fri, 03 Jun 2016 13:14:46 GMT
https://trac.sagemath.org/ticket/20705#comment:17
https://trac.sagemath.org/ticket/20705#comment:17
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/20705#comment:16" title="Comment 16">dlucas</a>:
Hi,
</p>
<p>
So should we keep <a class="missing wiki">BinaryReedMullerCode?</a> in codes_catalog.py in keeping with the <a class="missing wiki">BinaryReedMullerCode?</a>() function from guava.py? Or should i just keep the guava.py implementation with a warning regarding it's depreciation?
</p>
<blockquote class="citation">
<p>
Hello,
</p>
<p>
Some more comments:
</p>
<ul><li>I noticed you add both <code>ReedMullerCode</code>, <code>BinaryReedMullerCode</code> and <code>QAryReedMullerCode</code> to <code>codes_catalog.py</code>. But with our design, only <code>ReedMullerCode</code> should be in this file, as the user should never access the classes themselves.
</li></ul><h2 id="_multivariate_polynomial_interpolation"><code>_multivariate_polynomial_interpolation</code></h2>
<ul><li>You left a reference to <code>_R</code> in the docstring while it no longer exists
</li><li>Is the base field (and the cardinality) really needed? I would rather remove them from the list of inputs and recover them from <code>evaluation</code>. This way, one has to provide <code>evaluation</code> as a vector over a finite field, you get this finite field by doing <code>F = evaluation.base_ring()</code>, you check that <code>F</code> is the base ring of <code>polynomial_ring</code> and that's it. You can get <code>q</code> with the appropriate call on <code>F</code>.
</li><li>As my previous comment implies, I would only allow <code>evaluation</code> to be a vector (and not a list). After all, it's a private method you only use to recover the original message as a polynomial from a codeword, and the latter is necessarily a vector, isn't it?
</li><li>l. 105: is there a way to avoid a call to <code>base_field.list()</code>? I know that in most cases, this list will be rather small, but it's not really efficient to build a full list (especially in a recursive pattern, because you will build it a lot of times...). This time again, I'd suggest an iterator over the finite field.
</li></ul><h2 id="ReedMullerCode"><code>ReedMullerCode</code></h2>
<ul><li>the <code>WARNING</code> block should be added in the module documentation as well. The syntax on this block is the following
<pre class="wiki">.. WARNING::
body of the block
</pre></li></ul><p>
See <code>linear_code.py</code>, line 682 for an example.
I would also change the message. I propose <code>For now, this implementation only supports Reed-Muller codes whose order is less than q</code>. Such a message implies it will be extended later, I think it's more positive this way ;)
</p>
<h2 id="Q-aryandBinaryclasses">Q-ary and Binary classes</h2>
<ul><li>I think it's not necessary to save <code>q</code> as a class parameter. The user can access it by typing <code>C.base_field().cardinality()</code>, it's not something specific to the class (or something hard to "compute"), so it should not be saved.
</li></ul><ul><li>I prefer full names for user-accessed methods. I'm okay with <code>num_var</code> as an internal variable, but its getter should be called <code>number_of_variables</code> instead.
</li></ul><h2 id="Vectorialencoder">Vectorial encoder</h2>
<ul><li>In <code>generator_matrix</code>, you're calling <code>self.code()</code> a lot of times. I think you should store it in a local variable which will reduce the amount of calls to this method.
</li></ul><p>
Best,
</p>
<p>
David
</p>
</blockquote>
TicketdlucasFri, 03 Jun 2016 13:19:48 GMT
https://trac.sagemath.org/ticket/20705#comment:18
https://trac.sagemath.org/ticket/20705#comment:18
<blockquote class="citation">
<p>
So should we keep <a class="missing wiki">BinaryReedMullerCode?</a>? in codes_catalog.py in keeping with the <a class="missing wiki">BinaryReedMullerCode?</a>?() function from guava.py? Or should i just keep the guava.py implementation with a warning regarding it's depreciation?
</p>
</blockquote>
<p>
Oh, good point! Let's just keep <code>BinaryReedMullerCode</code> in <code>codes_catalog</code> for now, and we'll deal with it afterwards.
</p>
Ticketpanda314Fri, 03 Jun 2016 14:11:17 GMT
https://trac.sagemath.org/ticket/20705#comment:19
https://trac.sagemath.org/ticket/20705#comment:19
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/20705#comment:17" title="Comment 17">panda314</a>:
Hey so regarding <code>_multivariate_polynomial_interpolation</code>
so when i recursively use the function inside itself i actually pass a list and not a vector. Anycase since it's a private function does it matter?
</p>
TicketdlucasFri, 03 Jun 2016 14:21:10 GMT
https://trac.sagemath.org/ticket/20705#comment:20
https://trac.sagemath.org/ticket/20705#comment:20
<p>
Well, I'm just thinking about the future: maybe we can export that later on in another part of Sage.
But I guess the vector/list behaviour is ok for now, we can change it in the future if needed.
</p>
TicketgitFri, 03 Jun 2016 16:00:50 GMTcommit changed
https://trac.sagemath.org/ticket/20705#comment:21
https://trac.sagemath.org/ticket/20705#comment:21
<ul>
<li><strong>commit</strong>
changed from <em>5129c915c146098f2c9c03ce89ca992b32e86609</em> to <em>cd09b8a4b1b36e956077e99fc9d67003e998df40</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="http://git.sagemath.org/sage.git/commit/?id=649cead3036e939e4b0f799eb1ac1b3cc62308a6"><span class="icon"></span>649cead</a></td><td><code>removinng unecessary class parameters and variables and removing QAryReedMullerCode from codes_catalog.py and other tweaks</code>
</td></tr><tr><td><a class="ext-link" href="http://git.sagemath.org/sage.git/commit/?id=cd09b8a4b1b36e956077e99fc9d67003e998df40"><span class="icon"></span>cd09b8a</a></td><td><code>changing the warning messages</code>
</td></tr></table>
TicketgitSat, 04 Jun 2016 09:58:52 GMTcommit changed
https://trac.sagemath.org/ticket/20705#comment:22
https://trac.sagemath.org/ticket/20705#comment:22
<ul>
<li><strong>commit</strong>
changed from <em>cd09b8a4b1b36e956077e99fc9d67003e998df40</em> to <em>ad16c8f8dc11597d41fb4d933d4a02051bf29c01</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="http://git.sagemath.org/sage.git/commit/?id=ad16c8f8dc11597d41fb4d933d4a02051bf29c01"><span class="icon"></span>ad16c8f</a></td><td><code>corecting a misplaced warning message</code>
</td></tr></table>
TicketdlucasWed, 08 Jun 2016 08:47:52 GMT
https://trac.sagemath.org/ticket/20705#comment:23
https://trac.sagemath.org/ticket/20705#comment:23
<p>
Hello,
</p>
<p>
Some other remarks (the last ones from me):
</p>
<ul><li>in the input sanitization, you test integer parameters using this statement: <code>if not isinstance(param, Integer)</code>. Actually, as in Sage you can have both Sage Integers and Python ints, you need to write <code>if not isinstance(param, (int, Integer))</code> otherwise if the user passes a Python int for <code>param</code> it will be rejected by the check.
</li></ul><ul><li>in your <code>minimum_distance</code> method for <code>QAryReedMullerCode</code>, you should use integer division <code>//</code> instead of <code>/</code>. I agree the result will always be an integer because of the parameters, but using <code>/</code> returns a Rational while <code>//</code> returns an Integer, and I think it's better to return an Integer.
</li></ul><ul><li>method <code>generator_matrix</code> does not work if order is 0:
</li></ul><pre class="wiki">sage: sage: F = GF(3)
sage: C = codes.ReedMullerCode(F, 0, 2)
sage: sage: E = codes.encoders.ReedMullerVectorEncoder(C)
sage: sage: E.generator_matrix()
BOOM (StopIteration)
</pre><p>
In case of order 0, just return a matrix which has only one line of <code>1</code> (it is a repetition code in that case).
</p>
<ul><li>Still on generator matrix, you're using <code>exponents.first()</code> and <code>exponents.next()</code>. Why not using a "true" iterator instead? As you take the elements in <code>Subsets</code> one by one following the order they've been sorted, an Iterator has a better complexity in that case, because <code>next(obj)</code>'s complexity is <code>O(r)</code> where <code>r</code> is the rank of <code>obj</code>.
</li></ul><ul><li>Also, <code>generator_matrix</code> method is quite slow: with a RM code over <code>GF(16)</code>, order 14, 3 variables, it takes around 30s to compute its generator matrix (while with a GRS code over GF(4096), dimension 680, it takes around 3 seconds). It's just a remark so we keep it in mind, and I do not request any changes for this ticket. I'm completely fine with your implementation as is for a first ticket, and we can work on efficiency later on if we want to!
</li></ul><ul><li>Same with <code>encode</code> in <code>PolynomialEncoder</code>: some algorithms for fast multipoint evaluation exist, and we can use them to speed up this encoding. But once again, your implementation is definitely good enough for a first ticket!
</li></ul><p>
Otherwise, it sounds good. It's quite an impressive work for your first ticket, well done!
You will be the first one to have his implementation of an "advanced" linear code family in Sage. I'm jealous <code>;)</code>
</p>
<p>
David
</p>
TicketgitWed, 08 Jun 2016 09:48:59 GMTcommit changed
https://trac.sagemath.org/ticket/20705#comment:24
https://trac.sagemath.org/ticket/20705#comment:24
<ul>
<li><strong>commit</strong>
changed from <em>ad16c8f8dc11597d41fb4d933d4a02051bf29c01</em> to <em>260d8da2f65c87ecf0f3bb703608148684823d37</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="http://git.sagemath.org/sage.git/commit/?id=260d8da2f65c87ecf0f3bb703608148684823d37"><span class="icon"></span>260d8da</a></td><td><code>using iterators to loop through</code>
</td></tr></table>
Ticketpanda314Wed, 08 Jun 2016 09:57:59 GMT
https://trac.sagemath.org/ticket/20705#comment:25
https://trac.sagemath.org/ticket/20705#comment:25
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/20705#comment:23" title="Comment 23">dlucas</a>:
the error with order 0 was because '.next()' function was not computing in the last element of the subsets (a problem if the dimension becomes equal to the total number of possible exponents) anyways replaced with iterator so that problem is gone.
I was under the impression that .next() works like iterator. Anyways i used the python's iterator hopefully it will speed it up.
Speed does seem to be a big issue with this implementation. I am unable to lay a finger on the exact problem causing that.
</p>
<p>
Well i found reed solomon pretty advanced :D . Feels good to see 'Copyright (C) 2016 Parthasarathi Panda <parthasarathipanda314@…>' :) . Thanks for all the help. Next stop decoding
</p>
<blockquote class="citation">
<p>
Hello,
</p>
<p>
Some other remarks (the last ones from me):
</p>
<ul><li>in the input sanitization, you test integer parameters using this statement: <code>if not isinstance(param, Integer)</code>. Actually, as in Sage you can have both Sage Integers and Python ints, you need to write <code>if not isinstance(param, (int, Integer))</code> otherwise if the user passes a Python int for <code>param</code> it will be rejected by the check.
</li></ul><ul><li>in your <code>minimum_distance</code> method for <code>QAryReedMullerCode</code>, you should use integer division <code>//</code> instead of <code>/</code>. I agree the result will always be an integer because of the parameters, but using <code>/</code> returns a Rational while <code>//</code> returns an Integer, and I think it's better to return an Integer.
</li></ul><ul><li>method <code>generator_matrix</code> does not work if order is 0:
</li></ul><pre class="wiki">sage: sage: F = GF(3)
sage: C = codes.[wiki:ReedMullerCode](F, 0, 2)
sage: sage: E = codes.encoders.[wiki:ReedMullerVectorEncoder](C)
sage: sage: E.generator_matrix()
BOOM (StopIteration)
</pre><p>
In case of order 0, just return a matrix which has only one line of <code>1</code> (it is a repetition code in that case).
</p>
<ul><li>Still on generator matrix, you're using <code>exponents.first()</code> and <code>exponents.next()</code>. Why not using a "true" iterator instead? As you take the elements in <code>Subsets</code> one by one following the order they've been sorted, an Iterator has a better complexity in that case, because <code>next(obj)</code>'s complexity is <code>O(r)</code> where <code>r</code> is the rank of <code>obj</code>.
</li></ul><ul><li>Also, <code>generator_matrix</code> method is quite slow: with a RM code over <code>GF(16)</code>, order 14, 3 variables, it takes around 30s to compute its generator matrix (while with a GRS code over GF(4096), dimension 680, it takes around 3 seconds). It's just a remark so we keep it in mind, and I do not request any changes for this ticket. I'm completely fine with your implementation as is for a first ticket, and we can work on efficiency later on if we want to!
</li></ul><ul><li>Same with <code>encode</code> in <code>PolynomialEncoder</code>: some algorithms for fast multipoint evaluation exist, and we can use them to speed up this encoding. But once again, your implementation is definitely good enough for a first ticket!
</li></ul><p>
Otherwise, it sounds good. It's quite an impressive work for your first ticket, well done!
You will be the first one to have his implementation of an "advanced" linear code family in Sage. I'm jealous <code>;)</code>
</p>
<p>
David
</p>
</blockquote>
TicketdlucasWed, 08 Jun 2016 12:06:29 GMTbranch changed
https://trac.sagemath.org/ticket/20705#comment:26
https://trac.sagemath.org/ticket/20705#comment:26
<ul>
<li><strong>branch</strong>
changed from <em>u/panda314/classes_for_reed_muller_codes</em> to <em>u/dlucas/classes_for_reed_muller_codes</em>
</li>
</ul>
TicketdlucasWed, 08 Jun 2016 12:11:05 GMTcc, status, commit changed; reviewer set
https://trac.sagemath.org/ticket/20705#comment:27
https://trac.sagemath.org/ticket/20705#comment:27
<ul>
<li><strong>cc</strong>
<em>jsrn</em> <em>jlavauzelle</em> added
</li>
<li><strong>reviewer</strong>
set to <em>David Lucas</em>
</li>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
<li><strong>commit</strong>
changed from <em>260d8da2f65c87ecf0f3bb703608148684823d37</em> to <em>13cda90026b28644ed1c018d7628ea1e007cd4ef</em>
</li>
</ul>
<p>
Hello,
</p>
<p>
I made some minor changes to the documentation:
</p>
<ul><li>There was some syntax error which prevented it to compile properly, which I fixed
</li><li>I removed duplicated warning blocks
</li><li>I changed some line breaks
</li><li>I removed trailing whitespaces in the process
</li><li>I simplified a few methods' docstring.
</li></ul><p>
I did not change the code at all, just made some changes to the doc because it was faster to do it myself <code>:)</code>.
</p>
<p>
If you agree with my changes, we can give this ticket a positive review!
Don't forget to write your full name in the <code>Authors</code> field in this ticket.
</p>
<p>
Best,
</p>
<p>
David
</p>
<hr />
<p>
New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="http://git.sagemath.org/sage.git/commit/?id=77b533dd415dda89e303ac83bca90f29defca5e2"><span class="icon"></span>77b533d</a></td><td><code>Updated to 7.3beta3</code>
</td></tr><tr><td><a class="ext-link" href="http://git.sagemath.org/sage.git/commit/?id=13cda90026b28644ed1c018d7628ea1e007cd4ef"><span class="icon"></span>13cda90</a></td><td><code>Some tweaks (mostly documentation)</code>
</td></tr></table>
Ticketpanda314Wed, 08 Jun 2016 13:12:59 GMT
https://trac.sagemath.org/ticket/20705#comment:28
https://trac.sagemath.org/ticket/20705#comment:28
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/20705#comment:27" title="Comment 27">dlucas</a>:
Seems fine :)
</p>
<blockquote class="citation">
<blockquote>
<p>
Hello,
</p>
</blockquote>
<p>
</p>
<blockquote>
<p>
I made some minor changes to the documentation:
</p>
</blockquote>
<p>
</p>
<ul><li>There was some syntax error which prevented it to compile properly, which I fixed
</li><li>I removed duplicated warning blocks
</li><li>I changed some line breaks
</li><li>I removed trailing whitespaces in the process
</li><li>I simplified a few methods' docstring.
</li></ul><p>
</p>
<blockquote>
<p>
I did not change the code at all, just made some changes to the doc because it was faster to do it myself <code>:)</code>.
</p>
</blockquote>
<p>
</p>
<blockquote>
<p>
If you agree with my changes, we can give this ticket a positive review!
Don't forget to write your full name in the <code>Authors</code> field in this ticket.
</p>
</blockquote>
<p>
</p>
<blockquote>
<p>
Best,
</p>
</blockquote>
<p>
</p>
<blockquote>
<p>
David
----
New commits:
</p>
</blockquote>
<p>
</p>
<table class="wiki">
<tr><td> <a class="ext-link" href="http://git.sagemath.org/sage.git/commit/?id=77b533dd415dda89e303ac83bca90f29defca5e2"><span class="icon"></span>77b533d</a> </td><td> <code>Updated to 7.3beta3</code>
</td></tr></table>
<p>
</p>
<table class="wiki">
<tr><td> <a class="ext-link" href="http://git.sagemath.org/sage.git/commit/?id=13cda90026b28644ed1c018d7628ea1e007cd4ef"><span class="icon"></span>13cda90</a> </td><td> <code>Some tweaks (mostly documentation)</code>
</td></tr></table>
</blockquote>
Ticketpanda314Wed, 08 Jun 2016 13:14:37 GMTauthor set
https://trac.sagemath.org/ticket/20705#comment:29
https://trac.sagemath.org/ticket/20705#comment:29
<ul>
<li><strong>author</strong>
set to <em>Parthasarathi Panda</em>
</li>
</ul>
TicketjsrnThu, 09 Jun 2016 14:16:40 GMT
https://trac.sagemath.org/ticket/20705#comment:30
https://trac.sagemath.org/ticket/20705#comment:30
<p>
Test to see if I can comment.
</p>
TicketjsrnThu, 09 Jun 2016 14:19:15 GMT
https://trac.sagemath.org/ticket/20705#comment:31
https://trac.sagemath.org/ticket/20705#comment:31
<p>
I know I'm late to the scene, but I have a few questions/comments:
</p>
<ol><li>Why are lines 249--255 in reed_muller_code.py formatted so badly? Similarly for line 70--75. This type of formatting is not PEP8, I believe.
</li><li>The patch in <code>linear_code.py</code> adding <code>Integer</code> around <code>n-k</code> shouldn't be necessary: <code>n</code> is <code>C.length()</code> and <code>k</code> is <code>C.dimension()</code>. Both should *always* return <code>Integer</code>. You should find the example that led you to make this patch and instead fix the root cause that made either <code>n</code> or <code>k</code> rational. Incidentally, I think I know where the bug is: your <code>_binomial_sum</code> returns a rational since you have a division that you don't cast to <code>Integer</code>. Amusingly, that cast seems to speed up <code>_binomial_sum</code> by a factor 3 or so.
</li><li>Could you please improve the docstring of <code>order()</code> with a second sentence explaining what the order is? The current docstring is the type of infuriatingly unhelpful docstrings that are way too common ;-)
</li><li>Same for <code>number_of_variables</code> and <code>minimum_distance</code>. Yes, I am aware that there are code classes with similarly unhelpful docstrings, but let's try to improve Sage :-)
</li><li>For <code>__repr__</code> etc., perhaps it be more consistent with Reed--Solomon codes to write something like "Reed-Muller of order X in Y variables over <code>self.base_field()</code>".
</li></ol>
TicketjsrnThu, 09 Jun 2016 14:19:32 GMT
https://trac.sagemath.org/ticket/20705#comment:32
https://trac.sagemath.org/ticket/20705#comment:32
<ol start="6"><li>In <code>__eq__</code> you're checking equality of field cardinalities -- just check equality of the fields.
</li><li>In docstrings, use "Reed-Muller" consistently (and not "reed muller" or other things).
</li><li>In docstring warning of <code>ReedMullerCode</code>, the first sentence seems to be redundant (and not strictly true, since Reed-Muller codes make sense with order up to (d-1)*q). The same goes for the warning in <code>QaryReedMullerCode</code>. Also, all caps is ALWAYS ugly and impolite ;-)
</li><li>Can you please improve the documentation in <code>ReedMullerCode</code> to explain what a <code>ReedMullerCode</code> is? Also, the first paragraph of a docstring may only contain one sentence. Incidentally, in <code>ReedMullerCode</code>, I suggest removing the two other sentences in the first paragraph since that is an implementation detail and not relevant to the user.
</li><li>The docstring of <code>ReedMullerCode</code>, <code>num_of_var</code>: the <code>(i.e. m)</code> doesn't make sense to the user. Perhaps it will when you add the description of what a <code>ReedMullerCode</code> is.
</li><li>Perhaps add in the docstring of <code>QaryReedMullerCode</code> and <code>BinaryReedMullerCode</code> a reference to <code>ReedMullerCode</code> saying that this should usually be used, and also to see that function for description of the codes.
</li><li>There is some documentation missing about the order of the points used in <code>ReedMullerCode</code>, <code>_multivariate_polynomial_interpolation</code>, etc.
</li></ol>
TicketjsrnThu, 09 Jun 2016 14:20:14 GMT
https://trac.sagemath.org/ticket/20705#comment:33
https://trac.sagemath.org/ticket/20705#comment:33
<ol start="13"><li>Could you please add to the doctest of <code>_multivariate_polynomial_interpolation</code> that the returned polynomial interpolates the input values?
</li><li>Could you please add to the example blocks of <code>ReedMullerCode</code> something that prints the length and dimension and min-dist of the code? That is good for paedagogy.
</li><li>What happens in <code>_multivariate_polynomial_interpolation</code> if one inputs evaluations which cannot be interpolated by a polynomial of total degree <code>order</code>? Document.
</li><li>Consider using more mathematical docstring description rather than programming-like. See for instance <code>_multivariate_polynomial_interpolation</code>, where the first sentence could instead read something like `Return $f \in \GF(q)[X_1,...,X_m]$ such that $f(\mathbf a) = v[i(\mathbf a)]$ for all $\mathbf a \in \GF(q)<sup>m$, where $v \in GF(q)</sup>{q<sup>m}$ is a given vector of evaluations, and $i(a)$ is a specific ordering of $GF(q)</sup>m$ (see below for details)."
</li><li>The input <code>num_of_var</code> to <code>_multivariate_polynomial_interpolation</code> is redundant since you get the polynomial ring. Use <code>len(polynomial_ring.gens())</code> or something.
</li><li>In <code>_multivariate_polynomial_interpolation</code> you misspelled "coordinate".
</li><li>Instead of <code>Tuples(F.list(), m)</code> you should probably use <code>(F^m).list()</code>.
</li><li>Be careful with micro-optimisations that end up not having a real impact but makes the code denser to read. An example is the use of <code>z</code> and <code>x</code> in <code>_multivariate_polynomial_interpolation</code>. In line 117 you could also just write <code>polyVector += [base_field.zero()]*(d-len(polyVector))</code>.
</li><li>The doc of <code>_binomial_sum</code> sounds like it sums *up to and including <code>k</code>*. But it is only up to and including <code>k-1</code>.
</li></ol>
TicketjsrnThu, 09 Jun 2016 14:20:49 GMT
https://trac.sagemath.org/ticket/20705#comment:34
https://trac.sagemath.org/ticket/20705#comment:34
<ol start="22"><li>In <code>ReedMullerVectorEncoder.generator_matrix</code>, you should iterate directly over <code>exponents</code>: <code>for exponent in exponents: ...</code>.
</li><li>In <code>ReedMullerVectorEncoder.generator_matrix</code> and other places, perhaps <code>base_field_tuple</code> should simply be called <code>points</code>.
</li><li>There should be a method to get the list of points used for evaluation.
</li><li>I don't think <code>ReedMullerVectorEncoder.generator_matrix</code> is correct: you are indexing in the point <code>x</code> using <code>exponent</code>. But you *should* be computing <code>prod_{i=1}^m x[i]^exponent[i]</code>. Using <code>reduce(mul,...)</code> in this instance is less clear than using <code>prod(...)</code>.
</li><li>In doc for <code>ReedMullerPolynomialEncoder</code> it says "polynomial field", but it should be "polynomial ring". It should say that <code>polynomial_ring</code> is optional.
</li><li>Copy-paste error: it says something about <code>If code is not GRS code ...</code>.
</li><li>The polynomial in the example of <code>ReedMullerPolynomialEncoder.encode</code> is badly formatted.
</li><li>The failing example of <code>ReedMullerPolynomialEncoder.encode</code> with poly of too high degree could be with a poly where each variable has OK degree, but the total degree is bad, e.g. <code>x0^2*x1</code>.
</li></ol><p>
Otherwise, good job! Don't worry about the volume of my comments. David has fond memories of tickets where I similarly put him through the wringer ;-)
</p>
<p>
Best,
Johan
</p>
Ticketpanda314Thu, 09 Jun 2016 15:18:16 GMT
https://trac.sagemath.org/ticket/20705#comment:35
https://trac.sagemath.org/ticket/20705#comment:35
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/20705#comment:34" title="Comment 34">jsrn</a>:
No problem with comments :) . Will work on them. Some doubts/explanation.
</p>
<ol><li>Well i guess they are so because of the length of line restrictions and the automated formatter did everything 'strictly'. 249--255 is indeed ugly will change that. 70--75 seems fine though.
</li></ol><ol start="2"><li>Oh. I guess i will remove the linear_code.py path and use '<em>' instead of '/' in binomial sum. Might be faster too because of integer divisions instead of rational numbers.
</em></li></ol><ol start="21"><li>Isn't it sum upto 'k' that we need?
</li></ol><ol start="22"><li>Directly iterating involves enumeration over the subset redundantly if i am not mistaken. Used the iterator to do the generation in one scan.
</li></ol><ol start="24"><li>So given the term prod^m_{i=1} x^(d_i)_i, exponent is the set {{1}*d_1 <a class="report" href="https://trac.sagemath.org/report/2">{2}</a>*d_2 ...{m}*d_m}. In which case i believe that the generator matrix is correct (it matches with polynomial evaluation).
Will use the prod function instead.
</li></ol><p>
Well everything else makes sense. Will implement them :)
</p>
<blockquote class="citation">
<ol start="22"><li>In <code>ReedMullerVectorEncoder.generator_matrix</code>, you should iterate directly over <code>exponents</code>: <code>for exponent in exponents: ...</code>.
</li><li>In <code>ReedMullerVectorEncoder.generator_matrix</code> and other places, perhaps <code>base_field_tuple</code> should simply be called <code>points</code>.
</li><li>There should be a method to get the list of points used for evaluation.
</li><li>I don't think <code>ReedMullerVectorEncoder.generator_matrix</code> is correct: you are indexing in the point <code>x</code> using <code>exponent</code>. But you *should* be computing <code>prod_{i=1}^m x[i]^exponent[i]</code>. Using <code>reduce(mul,...)</code> in this instance is less clear than using <code>prod(...)</code>.
</li><li>In doc for <code>ReedMullerPolynomialEncoder</code> it says "polynomial field", but it should be "polynomial ring". It should say that <code>polynomial_ring</code> is optional.
</li><li>Copy-paste error: it says something about <code>If code is not GRS code ...</code>.
</li><li>The polynomial in the example of <code>ReedMullerPolynomialEncoder.encode</code> is badly formatted.
</li><li>The failing example of <code>ReedMullerPolynomialEncoder.encode</code> with poly of too high degree could be with a poly where each variable has OK degree, but the total degree is bad, e.g. <code>x0^2*x1</code>.
</li></ol><p>
Otherwise, good job! Don't worry about the volume of my comments. David has fond memories of tickets where I similarly put him through the wringer ;-)
</p>
<p>
Best,
Johan
</p>
</blockquote>
TicketjsrnThu, 09 Jun 2016 15:50:19 GMT
https://trac.sagemath.org/ticket/20705#comment:36
https://trac.sagemath.org/ticket/20705#comment:36
<blockquote class="citation">
<ol><li>Well i guess they are so because of the length of line restrictions and the automated formatter did everything 'strictly'. 249--255 is indeed ugly will change that. 70--75 seems fine though.
</li></ol></blockquote>
<p>
I won't be a stickler, so up to you. I looked up PEP8 and it doesn't seem to advocate putting parameters on individual lines.
</p>
<blockquote class="citation">
<ol start="2"><li>Oh. I guess i will remove the linear_code.py path and use '<em>' instead of '/' in binomial sum. Might be faster too because of integer divisions instead of rational numbers.
</em></li></ol></blockquote>
<p>
Yes.
</p>
<blockquote class="citation">
<ol start="21"><li>Isn't it sum upto 'k' that we need?
</li></ol></blockquote>
<p>
Yes it is. Just clarify in the docstring ("up to k" is ambiguous in English).
</p>
<blockquote class="citation">
<ol start="22"><li>Directly iterating involves enumeration over the subset redundantly if i am not mistaken. Used the iterator to do the generation in one scan.
</li></ol></blockquote>
<p>
Doing <code>for e in some_iterable</code> does exactly the same as looping with an iterator, memory-wise. What you shouldn't do is <code>for e in some_iterable.list()</code>.
</p>
<blockquote class="citation">
<ol start="24"><li>So given the term prod^m_{i=1} x^(d_i)_i, exponent is the set {{1}*d_1 <a class="report" href="https://trac.sagemath.org/report/2">{2}</a>*d_2 ...{m}*d_m}. In which case i believe that the generator matrix is correct (it matches with polynomial evaluation).
Will use the prod function instead.
</li></ol></blockquote>
<p>
Ah, yes now I see!
</p>
<p>
You should probably use <code>exponents = Subsets(..., k=order)</code>. Your current code
depends in a fragile, undocumented manner on the order that <code>Subsets</code> iterates
over its elements. When using <code>k=order</code> you get only subsets of size exactly
<code>k</code>, but you could get around this by adding a dummy element <code>order</code> times to
the list.
</p>
<blockquote class="citation">
<p>
Well everything else makes sense. Will implement them :)
</p>
</blockquote>
<p>
Cool!
</p>
Ticketpanda314Fri, 10 Jun 2016 06:17:54 GMT
https://trac.sagemath.org/ticket/20705#comment:37
https://trac.sagemath.org/ticket/20705#comment:37
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/20705#comment:33" title="Comment 33">jsrn</a>:
Regarding '17.' Is there any way one can pass a sub ring of a multivariate polynomial ring consisting of polynomials over a subset of the variables? Like F[x_1,x_2,x_3] is to F[x_1,x_2,x_3,x_4]? The num_of_var parameter used in the function sort of does that.
</p>
<p>
</p>
<blockquote class="citation">
<ol start="13"><li>Could you please add to the doctest of <code>_multivariate_polynomial_interpolation</code> that the returned polynomial interpolates the input values?
</li><li>Could you please add to the example blocks of <code>ReedMullerCode</code> something that prints the length and dimension and min-dist of the code? That is good for paedagogy.
</li><li>What happens in <code>_multivariate_polynomial_interpolation</code> if one inputs evaluations which cannot be interpolated by a polynomial of total degree <code>order</code>? Document.
</li><li>Consider using more mathematical docstring description rather than programming-like. See for instance <code>_multivariate_polynomial_interpolation</code>, where the first sentence could instead read something like `Return $f \in \GF(q)[X_1,...,X_m]$ such that $f(\mathbf a) = v[i(\mathbf a)]$ for all $\mathbf a \in \GF(q)<sup>m$, where $v \in GF(q)</sup>{q<sup>m}$ is a given vector of evaluations, and $i(a)$ is a specific ordering of $GF(q)</sup>m$ (see below for details)."
</li><li>The input <code>num_of_var</code> to <code>_multivariate_polynomial_interpolation</code> is redundant since you get the polynomial ring. Use <code>len(polynomial_ring.gens())</code> or something.
</li><li>In <code>_multivariate_polynomial_interpolation</code> you misspelled "coordinate".
</li><li>Instead of <code>Tuples(F.list(), m)</code> you should probably use <code>(F^m).list()</code>.
</li><li>Be careful with micro-optimisations that end up not having a real impact but makes the code denser to read. An example is the use of <code>z</code> and <code>x</code> in <code>_multivariate_polynomial_interpolation</code>. In line 117 you could also just write <code>polyVector += [base_field.zero()]*(d-len(polyVector))</code>.
</li><li>The doc of <code>_binomial_sum</code> sounds like it sums *up to and including <code>k</code>*. But it is only up to and including <code>k-1</code>.
</li></ol></blockquote>
Ticketpanda314Fri, 10 Jun 2016 08:07:59 GMT
https://trac.sagemath.org/ticket/20705#comment:38
https://trac.sagemath.org/ticket/20705#comment:38
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/20705#comment:32" title="Comment 32">jsrn</a>:
In '11.' is there any way to hyperlink the reference to <a class="missing wiki">ReedMullerCode?</a> in the reference of QAryReedMullerCode and <a class="missing wiki">BinaryReedMullerCode?</a>? Or shall i just write somehing like 'refer to documentation of' ?
</p>
<blockquote class="citation">
<ol start="6"><li>In <code>__eq__</code> you're checking equality of field cardinalities -- just check equality of the fields.
</li><li>In docstrings, use "Reed-Muller" consistently (and not "reed muller" or other things).
</li><li>In docstring warning of <code>ReedMullerCode</code>, the first sentence seems to be redundant (and not strictly true, since Reed-Muller codes make sense with order up to (d-1)*q). The same goes for the warning in <code>QaryReedMullerCode</code>. Also, all caps is ALWAYS ugly and impolite ;-)
</li><li>Can you please improve the documentation in <code>ReedMullerCode</code> to explain what a <code>ReedMullerCode</code> is? Also, the first paragraph of a docstring may only contain one sentence. Incidentally, in <code>ReedMullerCode</code>, I suggest removing the two other sentences in the first paragraph since that is an implementation detail and not relevant to the user.
</li><li>The docstring of <code>ReedMullerCode</code>, <code>num_of_var</code>: the <code>(i.e. m)</code> doesn't make sense to the user. Perhaps it will when you add the description of what a <code>ReedMullerCode</code> is.
</li><li>Perhaps add in the docstring of <code>QaryReedMullerCode</code> and <code>BinaryReedMullerCode</code> a reference to <code>ReedMullerCode</code> saying that this should usually be used, and also to see that function for description of the codes.
</li><li>There is some documentation missing about the order of the points used in <code>ReedMullerCode</code>, <code>_multivariate_polynomial_interpolation</code>, etc.
</li></ol></blockquote>
TicketdlucasFri, 10 Jun 2016 08:17:47 GMT
https://trac.sagemath.org/ticket/20705#comment:39
https://trac.sagemath.org/ticket/20705#comment:39
<blockquote class="citation">
<p>
In '11.' is there any way to hyperlink the reference to <a class="missing wiki">ReedMullerCode?</a>? in the reference of QAryReedMullerCode and <a class="missing wiki">BinaryReedMullerCode?</a>?? Or shall i just write somehing like 'refer to documentation of' ?
</p>
</blockquote>
<p>
You can write: <code>:class:<backquote>BinaryReedMullerCode<backquote></code>, it will create an hyperlink to <code>BinaryReedMullerCode</code>.
</p>
<p>
If you want to refer a class in another file in coding, you can use:
<code>:class:<backquote>sage.coding.file.ClassName<backquote></code>
</p>
<p>
And to hyperlink a method, use <code>:meth:</code> instead of <code>:class:</code>.
</p>
<p>
If you don't want to check manually if the links work, you can
use the command <code>./sage --docbuild --warn-links reference/coding</code> which will rebuild the documentation and tell you if some links are broken.
</p>
TicketjsrnFri, 10 Jun 2016 10:29:35 GMT
https://trac.sagemath.org/ticket/20705#comment:40
https://trac.sagemath.org/ticket/20705#comment:40
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/20705#comment:37" title="Comment 37">panda314</a>:
</p>
<blockquote class="citation">
<p>
Regarding '17.' Is there any way one can pass a sub ring of a multivariate polynomial ring consisting of polynomials over a subset of the variables? Like F[x_1,x_2,x_3] is to F[x_1,x_2,x_3,x_4]? The num_of_var parameter used in the function sort of does that.
</p>
</blockquote>
<p>
Ah yes, ok that's a good point. You *can* make such a sub-ring but perhaps that could get inefficient. Instead, you can restructure <code>_multivariate_polynomial_interpolation</code> with a local function:
</p>
<pre class="wiki">def _multivariate_polynomial_interpolation(evaluations, order, polynomial_ring):
def _interpolate(evaluations, order, num_of_var):
...
for k in range(d): # computing the polynomial
poly = poly + z * _interpolate([multipoint_evaluation_list[i][k] for i in range(n_by_q)],
order - k, num_of_var - 1)
z = z * x
return poly
return _interpolate(evaluations, order, len(polynomial_ring.gens()))
</pre>
Ticketpanda314Fri, 10 Jun 2016 10:54:28 GMTbranch changed
https://trac.sagemath.org/ticket/20705#comment:41
https://trac.sagemath.org/ticket/20705#comment:41
<ul>
<li><strong>branch</strong>
changed from <em>u/dlucas/classes_for_reed_muller_codes</em> to <em>u/panda314/classes_for_reed_muller_codes</em>
</li>
</ul>
Ticketpanda314Tue, 14 Jun 2016 03:00:28 GMTcommit changed
https://trac.sagemath.org/ticket/20705#comment:42
https://trac.sagemath.org/ticket/20705#comment:42
<ul>
<li><strong>commit</strong>
changed from <em>13cda90026b28644ed1c018d7628ea1e007cd4ef</em> to <em>5ec9c6c0a2fd6bdb3978a53d81b440efef41614a</em>
</li>
</ul>
<p>
Hi,
So have the changes been reviewed? Is the code fine?
</p>
<hr />
<p>
New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="http://git.sagemath.org/sage.git/commit/?id=5ec9c6c0a2fd6bdb3978a53d81b440efef41614a"><span class="icon"></span>5ec9c6c</a></td><td><code>rewriting doumentation, and rehformatting some part of the code</code>
</td></tr></table>
TicketjsrnTue, 14 Jun 2016 06:58:01 GMTreviewer changed
https://trac.sagemath.org/ticket/20705#comment:43
https://trac.sagemath.org/ticket/20705#comment:43
<ul>
<li><strong>reviewer</strong>
changed from <em>David Lucas</em> to <em>David Lucas, Johan S. R. Nielsen</em>
</li>
</ul>
<blockquote class="citation">
<p>
So have the changes been reviewed? Is the code fine?
</p>
</blockquote>
<p>
Please be patient :-) And for the record: when you push some commits to reply to a reviewer's concerns, please comment on your commit. At the very least, write something like "I fixed all the things you asked for. Ready for review again" or something.
</p>
<p>
I probably won't have time to go over it today. If David has time and is happy with your modifications to my comments, I'm sure that's fine.
</p>
TicketdlucasFri, 17 Jun 2016 11:18:27 GMTbranch changed
https://trac.sagemath.org/ticket/20705#comment:44
https://trac.sagemath.org/ticket/20705#comment:44
<ul>
<li><strong>branch</strong>
changed from <em>u/panda314/classes_for_reed_muller_codes</em> to <em>u/dlucas/classes_for_reed_muller_codes</em>
</li>
</ul>
TicketdlucasMon, 20 Jun 2016 18:49:22 GMTcommit changed
https://trac.sagemath.org/ticket/20705#comment:45
https://trac.sagemath.org/ticket/20705#comment:45
<ul>
<li><strong>commit</strong>
changed from <em>5ec9c6c0a2fd6bdb3978a53d81b440efef41614a</em> to <em>e3f81ec532cff5a5553f8abfddfc76c0a433c0e0</em>
</li>
</ul>
<p>
Hello,
</p>
<p>
It seems that my comment did not appear - sorry about that.
</p>
<p>
I fixed some small doc issues, rewrote a few docstrings, and removed a duplicated <code>WARNING</code> block.
</p>
<p>
If you're fine with my changes, you can give a positive review, everything is fine on my side.
</p>
<p>
Once again, well done with these codes :)
</p>
<p>
David
</p>
<hr />
<p>
New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="http://git.sagemath.org/sage.git/commit/?id=0f1f58e9025a32534020e7bc0359b0a5e954e275"><span class="icon"></span>0f1f58e</a></td><td><code>Merge branch 'u/panda314/classes_for_reed_muller_codes' of git://trac.sagemath.org/sage into classes_for_reed_muller_codes</code>
</td></tr><tr><td><a class="ext-link" href="http://git.sagemath.org/sage.git/commit/?id=e3f81ec532cff5a5553f8abfddfc76c0a433c0e0"><span class="icon"></span>e3f81ec</a></td><td><code>Fixed errors in documentation, rewrote some sentences, changed formatting, made some extra changes according to the other reviewer's comments</code>
</td></tr></table>
Ticketpanda314Tue, 21 Jun 2016 02:44:45 GMT
https://trac.sagemath.org/ticket/20705#comment:46
https://trac.sagemath.org/ticket/20705#comment:46
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/20705#comment:45" title="Comment 45">dlucas</a>:
tests are running fine. Seems cool :)
</p>
<blockquote class="citation">
<p>
Hello,
</p>
<p>
It seems that my comment did not appear - sorry about that.
</p>
<p>
I fixed some small doc issues, rewrote a few docstrings, and removed a duplicated <code>WARNING</code> block.
</p>
<p>
If you're fine with my changes, you can give a positive review, everything is fine on my side.
</p>
<p>
Once again, well done with these codes :)
</p>
<p>
David
</p>
<hr />
<p>
New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="http://git.sagemath.org/sage.git/commit/?id=0f1f58e9025a32534020e7bc0359b0a5e954e275"><span class="icon"></span>0f1f58e</a></td><td><code>Merge branch 'u/panda314/classes_for_reed_muller_codes' of git://trac.sagemath.org/sage into classes_for_reed_muller_codes</code>
</td></tr><tr><td><a class="ext-link" href="http://git.sagemath.org/sage.git/commit/?id=e3f81ec532cff5a5553f8abfddfc76c0a433c0e0"><span class="icon"></span>e3f81ec</a></td><td><code>Fixed errors in documentation, rewrote some sentences, changed formatting, made some extra changes according to the other reviewer's comments</code>
</td></tr></table>
</blockquote>
TicketjsrnTue, 21 Jun 2016 06:52:18 GMT
https://trac.sagemath.org/ticket/20705#comment:47
https://trac.sagemath.org/ticket/20705#comment:47
<p>
Then set it to positive_review as David coefficient said.
</p>
Ticketpanda314Tue, 21 Jun 2016 08:01:22 GMTstatus changed
https://trac.sagemath.org/ticket/20705#comment:48
https://trac.sagemath.org/ticket/20705#comment:48
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
</ul>
TicketvbraunThu, 23 Jun 2016 17:16:34 GMTstatus changed
https://trac.sagemath.org/ticket/20705#comment:49
https://trac.sagemath.org/ticket/20705#comment:49
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
Merge conflict, please merge in next beta.
</p>
TicketgitWed, 29 Jun 2016 08:28:27 GMTcommit changed
https://trac.sagemath.org/ticket/20705#comment:50
https://trac.sagemath.org/ticket/20705#comment:50
<ul>
<li><strong>commit</strong>
changed from <em>e3f81ec532cff5a5553f8abfddfc76c0a433c0e0</em> to <em>3cce07f24554b489738c721584a689df65d8655a</em>
</li>
</ul>
<p>
Branch pushed to git repo; I updated commit sha1. New commits:
</p>
<table class="wiki">
<tr><td><a class="ext-link" href="http://git.sagemath.org/sage.git/commit/?id=3cce07f24554b489738c721584a689df65d8655a"><span class="icon"></span>3cce07f</a></td><td><code>Updated to latest beta and fixed conflicts</code>
</td></tr></table>
TicketdlucasWed, 29 Jun 2016 08:29:33 GMTstatus changed
https://trac.sagemath.org/ticket/20705#comment:51
https://trac.sagemath.org/ticket/20705#comment:51
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
<blockquote class="citation">
<p>
Merge conflict, please merge in next beta.
</p>
</blockquote>
<p>
Done, ticket open for review.
</p>
<p>
David
</p>
Ticketpanda314Wed, 29 Jun 2016 12:33:05 GMT
https://trac.sagemath.org/ticket/20705#comment:52
https://trac.sagemath.org/ticket/20705#comment:52
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/20705#comment:51" title="Comment 51">dlucas</a>:
</p>
<blockquote class="citation">
<blockquote class="citation">
<p>
Merge conflict, please merge in next beta.
</p>
</blockquote>
</blockquote>
<p>
Seems fine. Tests passed.
</p>
<blockquote class="citation">
<p>
Done, ticket open for review.
</p>
<p>
David
</p>
</blockquote>
TicketdlucasWed, 29 Jun 2016 15:02:31 GMT
https://trac.sagemath.org/ticket/20705#comment:53
https://trac.sagemath.org/ticket/20705#comment:53
<blockquote class="citation">
<p>
Seems fine. Tests passed.
</p>
</blockquote>
<p>
Cool!
</p>
<p>
Can you please set it to <code>positive_review</code> then?
</p>
Ticketpanda314Wed, 29 Jun 2016 15:06:56 GMTstatus changed
https://trac.sagemath.org/ticket/20705#comment:54
https://trac.sagemath.org/ticket/20705#comment:54
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
</ul>
TicketvbraunThu, 30 Jun 2016 12:20:55 GMTstatus, branch changed; resolution set
https://trac.sagemath.org/ticket/20705#comment:55
https://trac.sagemath.org/ticket/20705#comment:55
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>closed</em>
</li>
<li><strong>resolution</strong>
set to <em>fixed</em>
</li>
<li><strong>branch</strong>
changed from <em>u/dlucas/classes_for_reed_muller_codes</em> to <em>3cce07f24554b489738c721584a689df65d8655a</em>
</li>
</ul>
TicketjdemeyerTue, 16 Aug 2016 11:31:57 GMTreviewer changed; commit deleted
https://trac.sagemath.org/ticket/20705#comment:56
https://trac.sagemath.org/ticket/20705#comment:56
<ul>
<li><strong>reviewer</strong>
changed from <em>David Lucas, Johan S. R. Nielsen</em> to <em>David Lucas, Johan Sebastian Rosenkilde Nielsen</em>
</li>
<li><strong>commit</strong>
<em>3cce07f24554b489738c721584a689df65d8655a</em> deleted
</li>
</ul>
Ticket