Opened 5 years ago
Closed 5 years ago
#20744 closed enhancement (fixed)
Polynomial encoder for GRS codes fails if variable name is not x
Reported by:  dlucas  Owned by:  

Priority:  major  Milestone:  sage7.4 
Component:  coding theory  Keywords:  sd75 
Cc:  jsrn, jlavauzelle, panda314  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 5 years ago by
 Branch set to u/dlucas/grs_polynomial_encoder_msg_space_fix
comment:2 Changed 5 years ago by
 Commit set to 9c3c1cde84fa9766f1335222d9a09695e5f99220
 Status changed from new to needs_review
comment:3 Changed 5 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 5 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 5 years ago by
 Commit changed from 9c3c1cde84fa9766f1335222d9a09695e5f99220 to 7692498a270d5847ff5987a91f9d4afe30696578
comment:6 followup: ↓ 7 Changed 5 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 in reply to: ↑ 6 Changed 5 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 5 years ago by
 Commit changed from 7692498a270d5847ff5987a91f9d4afe30696578 to 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 5 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 5 years ago by
 Reviewers set to 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 5 years ago by
 Commit changed from 08602206970b3624729f42f73bf7f71c6df02817 to 6c5d8ff98caccd4e90738efcf9121afae98b58c1
comment:12 Changed 5 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 5 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 5 years ago by
 Branch changed from u/dlucas/grs_polynomial_encoder_msg_space_fix to u/jlavauzelle/grs_polynomial_encoder_msg_space_fix
comment:15 Changed 5 years ago by
 Branch changed from u/jlavauzelle/grs_polynomial_encoder_msg_space_fix to u/dlucas/grs_polynomial_encoder_msg_space_fix
 Milestone changed from sage7.3 to 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
comment:16 Changed 5 years ago by
 Branch changed from u/dlucas/grs_polynomial_encoder_msg_space_fix to u/jlavauzelle/grs_polynomial_encoder_msg_space_fix
comment:17 Changed 5 years ago by
 Commit changed from 6c5d8ff98caccd4e90738efcf9121afae98b58c1 to 05bd5ec0d3cb7242e2d63650195ebfa175dd5418
 Status changed from needs_review to needs_work
comment:18 Changed 5 years ago by
 Commit changed from 05bd5ec0d3cb7242e2d63650195ebfa175dd5418 to e5fef364dd43ce2b8417aa286f2c72b89812468e
Branch pushed to git repo; I updated commit sha1. New commits:
e5fef36  Fixed __eq__ issue related to polynomial_ring.

comment:19 Changed 5 years ago by
Hi Johan,
That's true, sorry. Fixed.
Julien
comment:20 Changed 5 years ago by
 Status changed from needs_work to needs_review
comment:21 Changed 5 years ago by
 Keywords sd75 added
 Reviewers changed from Johan S. R. Nielsen to Johan Rosenkilde, Daniel Augot
 Status changed from needs_review to positive_review
comment:22 Changed 5 years ago by
 Branch changed from u/jlavauzelle/grs_polynomial_encoder_msg_space_fix to e5fef364dd43ce2b8417aa286f2c72b89812468e
 Resolution set to fixed
 Status changed from positive_review to 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