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:  sage7.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: 
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
Branch:  → u/dlucas/grs_polynomial_encoder_msg_space_fix 

comment:2 Changed 7 years ago by
Commit:  → 9c3c1cde84fa9766f1335222d9a09695e5f99220 

Status:  new → needs_review 
comment:3 Changed 7 years ago by
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
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
Commit:  9c3c1cde84fa9766f1335222d9a09695e5f99220 → 7692498a270d5847ff5987a91f9d4afe30696578 

comment:6 followup: 7 Changed 7 years ago by
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 überclean, I can live with that.
David
comment:7 Changed 7 years ago by
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 withmessage_space
, but it makes sense to provide it, so even if it's not überclean, 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 docstring that this is the same as the message space?
comment:8 Changed 7 years ago by
Commit:  7692498a270d5847ff5987a91f9d4afe30696578 → 08602206970b3624729f42f73bf7f71c6df02817 

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

comment:9 Changed 7 years ago by
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
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
Commit:  08602206970b3624729f42f73bf7f71c6df02817 → 6c5d8ff98caccd4e90738efcf9121afae98b58c1 

comment:12 Changed 6 years ago by
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
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
Branch:  u/dlucas/grs_polynomial_encoder_msg_space_fix → u/jlavauzelle/grs_polynomial_encoder_msg_space_fix 

comment:15 Changed 6 years ago by
Branch:  u/jlavauzelle/grs_polynomial_encoder_msg_space_fix → u/dlucas/grs_polynomial_encoder_msg_space_fix 

Milestone:  sage7.3 → sage7.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
comment:16 Changed 6 years ago by
Branch:  u/dlucas/grs_polynomial_encoder_msg_space_fix → u/jlavauzelle/grs_polynomial_encoder_msg_space_fix 

comment:17 Changed 6 years ago by
Commit:  6c5d8ff98caccd4e90738efcf9121afae98b58c1 → 05bd5ec0d3cb7242e2d63650195ebfa175dd5418 

Status:  needs_review → needs_work 
comment:18 Changed 6 years ago by
Commit:  05bd5ec0d3cb7242e2d63650195ebfa175dd5418 → e5fef364dd43ce2b8417aa286f2c72b89812468e 

Branch pushed to git repo; I updated commit sha1. New commits:
e5fef36  Fixed __eq__ issue related to polynomial_ring.

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

comment:21 Changed 6 years ago by
Authors:  → Julien Lavauzelle, David Lucas 

Keywords:  sd75 added 
Reviewers:  Johan S. R. Nielsen → Johan Rosenkilde, Daniel Augot 
Status:  needs_review → positive_review 
comment:22 Changed 6 years ago by
Branch:  u/jlavauzelle/grs_polynomial_encoder_msg_space_fix → e5fef364dd43ce2b8417aa286f2c72b89812468e 

Resolution:  → fixed 
Status:  positive_review → closed 
I fixed the bug, this is now open for review.
Best,
David
New commits:
Fixe d bug related to hardcoded variable for the polynomial ring