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

[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.

[dlucas]

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

[jsrn]

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().

[dlucas]

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().

[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()

[dlucas]

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

[jsrn]

Replying to 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?

[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

[dlucas]

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.

[jsrn]

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

[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)

[dlucas]

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

[danielaugot]

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]

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

[jsrn]

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.

[git]

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

​e5fef36

Fixed __eq__ issue related to polynomial_ring.

[jlavauzelle]

Hi Johan,

That's true, sorry. Fixed.

Julien