Opened 6 years ago
Closed 6 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: | sage-7.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 6 years ago by
- Branch set to u/dlucas/grs_polynomial_encoder_msg_space_fix
comment:2 Changed 6 years ago by
- Commit set to 9c3c1cde84fa9766f1335222d9a09695e5f99220
- Status changed from new to needs_review
comment:3 Changed 6 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 6 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 6 years ago by
- Commit changed from 9c3c1cde84fa9766f1335222d9a09695e5f99220 to 7692498a270d5847ff5987a91f9d4afe30696578
comment:6 follow-up: ↓ 7 Changed 6 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 über-clean, I can live with that.
David
comment:7 in reply to: ↑ 6 Changed 6 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 ü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 6 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 6 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 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 6 years ago by
- Commit changed from 08602206970b3624729f42f73bf7f71c6df02817 to 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 changed from u/dlucas/grs_polynomial_encoder_msg_space_fix to u/jlavauzelle/grs_polynomial_encoder_msg_space_fix
comment:15 Changed 6 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 sage-7.3 to sage-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
comment:16 Changed 6 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 6 years ago by
- Commit changed from 6c5d8ff98caccd4e90738efcf9121afae98b58c1 to 05bd5ec0d3cb7242e2d63650195ebfa175dd5418
- Status changed from needs_review to needs_work
comment:18 Changed 6 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 6 years ago by
Hi Johan,
That's true, sorry. Fixed.
Julien
comment:20 Changed 6 years ago by
- Status changed from needs_work to needs_review
comment:21 Changed 6 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 6 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