Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug in Reed-Solomon encoder and error-erasure decoder #30045

Closed
johanrosenkilde opened this issue Jul 2, 2020 · 23 comments
Closed

Bug in Reed-Solomon encoder and error-erasure decoder #30045

johanrosenkilde opened this issue Jul 2, 2020 · 23 comments

Comments

@johanrosenkilde
Copy link
Contributor

The following two snippets demonstrate two bugs when working with GeneralizedReedSolomonCode:

sage: F = GF(59)
sage: n, k = 40, 12
sage: C = codes.GeneralizedReedSolomonCode(F.list()[:n], k)
sage: D = codes.decoders.GRSErrorErasureDecoder(C)
sage: y = (vector(F, [0, 0, 10, 0, 0, 22, 0, 0, 38, 8, 34, 14, 33, 0, 0, 39, 0, 0, 0, 0, 17, 36, 43, 30, 10, 15, 0, 0, 21, 10, 37, 0, 0, 0, 0, 0, 0, 0, 0, 42]),
sage:      vector(GF(2), [1, 1, 0, 1, 1, 0, 1, 1, 0, 0, 0, 0, 0, 1, 1, 0, 1, 1, 1, 1, 0, 0, 0, 0, 0, 0, 1, 1, 0, 0, 0, 1, 1, 1, 1, 1, 1, 1, 1, 0]))
sage: D.decode_to_code(y)
<BOOM>

Decoding to message works, but produces a vector of incorrect length (should equal the dimension, i.e. 12):

sage: m = D.decode_to_message(y)
sage: len(m)
11

(The bugs were found by introducing random seeding in #29945)

CC: @kliem

Component: coding theory

Keywords: reed-solomon decoding

Author: Johan Rosenkilde

Branch/Commit: b1fa133

Reviewer: Jonathan Kliem

Issue created by migration from https://trac.sagemath.org/ticket/30045

@johanrosenkilde
Copy link
Contributor Author

comment:1

The second error seems to be due to GeneralizedReedSolomonCode having a decode_to_message implemented, which incorrectly converts between polynomials and vectors. I don't see why it should have this method implemented - it should be provided in the correct way by AbstractCode.

@johanrosenkilde

This comment has been minimized.

@johanrosenkilde
Copy link
Contributor Author

Dependencies: #29945

@johanrosenkilde
Copy link
Contributor Author

Branch: u/jsrn/30045_bugs_in_grs

@kliem
Copy link
Contributor

kliem commented Jul 6, 2020

Commit: 18428ed

@kliem
Copy link
Contributor

kliem commented Jul 6, 2020

comment:4

As posted already on #29945, I would propose to just fix the problem and don't worry about #29945.


New commits:

18428edRemove unnecessary decode_to_message in GeneralizedReedSolomonCode

@johanrosenkilde
Copy link
Contributor Author

comment:5

OK, then this is up for review.

@johanrosenkilde
Copy link
Contributor Author

Changed dependencies from #29945 to none

@kliem
Copy link
Contributor

kliem commented Jul 6, 2020

comment:6

The bug fix should be tested with a doctest yet.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 6, 2020

Changed commit from 18428ed to 36cff4a

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 6, 2020

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

36cff4aAdd doc-test for 30045

@johanrosenkilde
Copy link
Contributor Author

comment:8

Right you are.

@kliem
Copy link
Contributor

kliem commented Jul 8, 2020

comment:9

Can you please add the doctest that was removed with the method somewhere.

Also

-    Test that the bug in #30045 is fixed::
+    Test that the bug in :trac:`30045` is fixed::

@kliem
Copy link
Contributor

kliem commented Jul 8, 2020

Reviewer: Jonathan Kliem

@kliem
Copy link
Contributor

kliem commented Jul 10, 2020

comment:10

And missing author name. Otherwise this looks good.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 10, 2020

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

f7ae23bReinstate deleted doctest/example in a meaningful place

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 10, 2020

Changed commit from 36cff4a to f7ae23b

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 10, 2020

Changed commit from f7ae23b to b1fa133

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 10, 2020

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

b1fa133Fix trac ticket link

@johanrosenkilde
Copy link
Contributor Author

Author: Johan Rosenkilde

@johanrosenkilde
Copy link
Contributor Author

comment:13

Thanks for the patience! It's been a while since I last fixed something in Sage ;-)

@kliem
Copy link
Contributor

kliem commented Jul 10, 2020

comment:14

LGTM. Thanks for fixing this.

@vbraun
Copy link
Member

vbraun commented Jul 12, 2020

Changed branch from u/jsrn/30045_bugs_in_grs to b1fa133

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants