Opened 6 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: 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:

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 6 years ago by dlucas

  • Branch set to u/dlucas/grs_polynomial_encoder_msg_space_fix

comment:2 Changed 6 years ago by dlucas

  • Commit set to 9c3c1cde84fa9766f1335222d9a09695e5f99220
  • Status changed from new to needs_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 6 years ago by 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().

comment:4 Changed 6 years ago by 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().

comment:5 Changed 6 years ago by git

  • Commit changed from 9c3c1cde84fa9766f1335222d9a09695e5f99220 to 7692498a270d5847ff5987a91f9d4afe30696578

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 follow-up: Changed 6 years ago by 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

comment:7 in reply to: ↑ 6 Changed 6 years ago by 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?

comment:8 Changed 6 years ago by git

  • Commit changed from 7692498a270d5847ff5987a91f9d4afe30696578 to 08602206970b3624729f42f73bf7f71c6df02817

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 6 years ago by 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.

comment:10 Changed 5 years ago by jsrn

  • 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 git

  • Commit changed from 08602206970b3624729f42f73bf7f71c6df02817 to 6c5d8ff98caccd4e90738efcf9121afae98b58c1

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 5 years ago by 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

comment:13 Changed 5 years ago by 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

comment:14 Changed 5 years ago by jlavauzelle

  • 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 jlavauzelle

  • 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

Last edited 5 years ago by jlavauzelle (previous) (diff)

comment:16 Changed 5 years ago by jlavauzelle

  • 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 jsrn

  • Commit changed from 6c5d8ff98caccd4e90738efcf9121afae98b58c1 to 05bd5ec0d3cb7242e2d63650195ebfa175dd5418
  • Status changed from needs_review to needs_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 5 years ago by git

  • Commit changed from 05bd5ec0d3cb7242e2d63650195ebfa175dd5418 to e5fef364dd43ce2b8417aa286f2c72b89812468e

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

e5fef36Fixed __eq__ issue related to polynomial_ring.

comment:19 Changed 5 years ago by jlavauzelle

Hi Johan,

That's true, sorry. Fixed.

Julien

comment:20 Changed 5 years ago by jlavauzelle

  • Status changed from needs_work to needs_review

comment:21 Changed 5 years ago by jsrn

  • Authors set to Julien Lavauzelle, David Lucas
  • 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 vbraun

  • Branch changed from u/jlavauzelle/grs_polynomial_encoder_msg_space_fix to e5fef364dd43ce2b8417aa286f2c72b89812468e
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.