Polynomial encoder for GRS codes fails if variable name is not x

[sagetrac-dlucas]

The following:

sage: F = GF(11)
sage: Fy.<y> = F[]
sage: n, k = 10 , 5
sage: C = codes.GeneralizedReedSolomonCode(F.list()[:n], k)
sage: E = C.encoder("EvaluationPolynomial")
sage: p = y^2 + 3*y + 10
sage: c = E.encode(p);

<BOOM>

fails because the polynomial to encode has to be in 'x' with the actual implementation.

This ticket fixes it, by allowing the user to specify a variable name when creating the encoder.

CC: @johanrosenkilde @jlavauzelle @sagetrac-panda314

Component: coding theory

Keywords: sd75

Author: Julien Lavauzelle, David Lucas

Branch/Commit: e5fef36

Reviewer: Johan Rosenkilde, Daniel Augot

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

[sagetrac-dlucas]

Branch: u/dlucas/grs_polynomial_encoder_msg_space_fix

[sagetrac-dlucas]

comment:2

I fixed the bug, this is now open for review.

Best,

David

---

New commits:

9c3c1cd

Fixe d bug related to hardcoded variable for the polynomial ring

[sagetrac-dlucas]

Commit: 9c3c1cd

[johanrosenkilde]

comment:3

I was wondering whether it would make more sense to have an option to give the polynomial ring? But I'm not sure it would. For ReedMullerCodes (which are in the making), one could argue that it does make sense (since we otherwise need many names), and in this case, it would be consistent that ReedSolomonCode follows the same pattern.

One could also, for usability, make a method polynomial_ring() on the encoder which is an alias of message_space().

[sagetrac-dlucas]

comment:4

Yes, I think it's important to be consistent between similar modules.
I'll change this so it now asks for a polynomial ring and not the variable.
I'll also add a method polynomial_ring().

[sagetrac-git]

Changed commit from 9c3c1cd to 7692498

[sagetrac-git]

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

99e9cd1	Instead of the variable name, the encoder now takes the ring as optional input
7692498	Added a getter method: polynomial_ring()

[sagetrac-dlucas]

comment:6

I did what we talked about, and added related input checks and TESTS block.
This method polynomial_ring feels like code duplication with message_space, but it makes sense to provide it, so even if it's not über-clean, I can live with that.

David

[johanrosenkilde]

comment:7

Replying to @sagetrac-dlucas:

I did what we talked about, and added related input checks and TESTS block.

Cool. You also need a check that polynomial_ring.base_ring() is the field of the code.

This method polynomial_ring feels like code duplication with message_space, but it makes sense to provide it, so even if it's not über-clean, I can live with that.

Yes, that's why I suggested making it an "alias of message_space". But now you've made a full function anyway. However, can you add in the doc-string that this is the same as the message space?

[sagetrac-git]

Changed commit from 7692498 to 0860220

[sagetrac-git]

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

0860220

Added another input check, polynomial_ring is now an alias of message_space

[sagetrac-dlucas]

comment:9

Done.

I also removed polynomial_ring as a full function and made it an alias of message_space as you suggested.
I think it's indeed better.

[johanrosenkilde]

Reviewer: Johan S. R. Nielsen

[johanrosenkilde]

comment:10

This looks good. I approve of the changes. However, I can't currently compile Sage on my machine, so I can't really test the patch :-S

[sagetrac-git]

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

bffcbef	Merge branch 'develop' into grs_polynomial_encoder_msg_space_fix
6c5d8ff	Fixed bug (confusion between base_ring and base_field)

[sagetrac-git]

Changed commit from 0860220 to 6c5d8ff

[sagetrac-dlucas]

comment:12

I updated this ticket to latest beta, and fixed a bug (polynomial_ring.base_field() was used instead of polynomial_ring.base_ring()).

This is still open for review.

David

[sagetrac-danielaugot]

comment:13

Hi David

This new feature is not examplified when typing codes.encoders.GRSEvaluationPolynomialEncoder?

I suggest there should be an example with 'y' or 'my_variable' in the EXAMPLE section of the docstring.

That could be quickly done by migrating part of the existing code in the TEST section to the EXAMPLE section.

Daniel

[jlavauzelle]

Changed branch from u/dlucas/grs_polynomial_encoder_msg_space_fix to u/jlavauzelle/grs_polynomial_encoder_msg_space_fix

[jlavauzelle]

Changed branch from u/jlavauzelle/grs_polynomial_encoder_msg_space_fix to u/dlucas/grs_polynomial_encoder_msg_space_fix

[jlavauzelle]

comment:15

Hi,

I merged to the latest beta (7.4.beta3) and made the fix according to Daniel's comment. I also approve the changes.

Need someone for a definitive review (I guess).

Julien

[jlavauzelle]

Changed branch from u/dlucas/grs_polynomial_encoder_msg_space_fix to u/jlavauzelle/grs_polynomial_encoder_msg_space_fix

[johanrosenkilde]

Changed commit from 6c5d8ff to 05bd5ec

[johanrosenkilde]

comment:17

The polynomial ring has to go into __eq__.

---

New commits:

4e3cbbc	Merge branch 'u/dlucas/grs_polynomial_encoder_msg_space_fix' of git://trac.sagemath.org/sage into 20744_grs_variable_name
05bd5ec	Improved doc and tests related to polynomial_ring argument.

[sagetrac-git]

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

e5fef36

Fixed `__eq__` issue related to polynomial_ring.

[sagetrac-git]

Changed commit from 05bd5ec to e5fef36

[jlavauzelle]

comment:19

Hi Johan,

That's true, sorry. Fixed.

Julien

[johanrosenkilde]

Changed keywords from none to sd75

[johanrosenkilde]

Author: Julien Lavauzelle, David Lucas

[johanrosenkilde]

Changed reviewer from Johan S. R. Nielsen to Johan Rosenkilde, Daniel Augot

[vbraun]

Changed branch from u/jlavauzelle/grs_polynomial_encoder_msg_space_fix to e5fef36