Opened 7 years ago

Closed 6 years ago

#20744 closed enhancement (fixed)

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

Reported by: David Lucas Owned by:
Priority: major Milestone: sage-7.4
Component: coding theory Keywords: sd75
Cc: Johan Rosenkilde, Julien Lavauzelle, Parthasarathi Panda Merged in:
Authors: Julien Lavauzelle, David Lucas Reviewers: Johan Rosenkilde, Daniel Augot
Report Upstream: N/A Work issues:
Branch: e5fef36 (Commits, GitHub, GitLab) Commit: e5fef364dd43ce2b8417aa286f2c72b89812468e
Dependencies: Stopgaps:

Status badges

Description

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.

Change History (22)

comment:1 Changed 7 years ago by David Lucas

Branch: u/dlucas/grs_polynomial_encoder_msg_space_fix

comment:2 Changed 7 years ago by David Lucas

Commit: 9c3c1cde84fa9766f1335222d9a09695e5f99220
Status: newneeds_review

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

Best,

David


New commits:

9c3c1cdFixe d bug related to hardcoded variable for the polynomial ring

comment:3 Changed 7 years ago by Johan Rosenkilde

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

comment:4 Changed 7 years ago by David Lucas

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

comment:5 Changed 7 years ago by git

Commit: 9c3c1cde84fa9766f1335222d9a09695e5f992207692498a270d5847ff5987a91f9d4afe30696578

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

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

comment:6 Changed 7 years ago by David Lucas

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

comment:7 in reply to:  6 Changed 7 years ago by Johan Rosenkilde

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?

comment:8 Changed 7 years ago by git

Commit: 7692498a270d5847ff5987a91f9d4afe3069657808602206970b3624729f42f73bf7f71c6df02817

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

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

comment:9 Changed 7 years ago by David Lucas

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.

comment:10 Changed 6 years ago by Johan Rosenkilde

Reviewers: Johan S. R. Nielsen

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

comment:11 Changed 6 years ago by git

Commit: 08602206970b3624729f42f73bf7f71c6df028176c5d8ff98caccd4e90738efcf9121afae98b58c1

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

bffcbefMerge branch 'develop' into grs_polynomial_encoder_msg_space_fix
6c5d8ffFixed bug (confusion between base_ring and base_field)

comment:12 Changed 6 years ago by David Lucas

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

comment:13 Changed 6 years ago by Daniel Augot

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

comment:14 Changed 6 years ago by Julien Lavauzelle

Branch: u/dlucas/grs_polynomial_encoder_msg_space_fixu/jlavauzelle/grs_polynomial_encoder_msg_space_fix

comment:15 Changed 6 years ago by Julien Lavauzelle

Branch: u/jlavauzelle/grs_polynomial_encoder_msg_space_fixu/dlucas/grs_polynomial_encoder_msg_space_fix
Milestone: sage-7.3sage-7.4

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

Edit: weirdly the push didn't work

Version 1, edited 6 years ago by Julien Lavauzelle (previous) (next) (diff)

comment:16 Changed 6 years ago by Julien Lavauzelle

Branch: u/dlucas/grs_polynomial_encoder_msg_space_fixu/jlavauzelle/grs_polynomial_encoder_msg_space_fix

comment:17 Changed 6 years ago by Johan Rosenkilde

Commit: 6c5d8ff98caccd4e90738efcf9121afae98b58c105bd5ec0d3cb7242e2d63650195ebfa175dd5418
Status: needs_reviewneeds_work

The polynomial ring has to go into __eq__.


New commits:

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

comment:18 Changed 6 years ago by git

Commit: 05bd5ec0d3cb7242e2d63650195ebfa175dd5418e5fef364dd43ce2b8417aa286f2c72b89812468e

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

e5fef36Fixed __eq__ issue related to polynomial_ring.

comment:19 Changed 6 years ago by Julien Lavauzelle

Hi Johan,

That's true, sorry. Fixed.

Julien

comment:20 Changed 6 years ago by Julien Lavauzelle

Status: needs_workneeds_review

comment:21 Changed 6 years ago by Johan Rosenkilde

Authors: Julien Lavauzelle, David Lucas
Keywords: sd75 added
Reviewers: Johan S. R. NielsenJohan Rosenkilde, Daniel Augot
Status: needs_reviewpositive_review

comment:22 Changed 6 years ago by Volker Braun

Branch: u/jlavauzelle/grs_polynomial_encoder_msg_space_fixe5fef364dd43ce2b8417aa286f2c72b89812468e
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.