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

__call__(message) on a linear code or encoder should return a codeword. #20087

Closed
johanrosenkilde opened this issue Feb 19, 2016 · 51 comments
Closed

Comments

@johanrosenkilde
Copy link
Contributor

If E is an Encoder for a LinearCode C, one can use E.encode(message), where message is in the appropriate space. E(message) should default to the same behaviour. So should C(message).

Be aware that the category framework specifies that C(c) = c if c in C. This needs to be handled.

CC: @sagetrac-dlucas @ClementPernet @jlavauzelle @sagetrac-danielaugot

Component: coding theory

Keywords: rd3

Author: David Lucas

Branch/Commit: 8ba64ee

Reviewer: Clément Pernet

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

@sagetrac-dlucas
Copy link
Mannequin

sagetrac-dlucas mannequin commented Feb 22, 2016

Branch: u/dlucas/shortcut_to_encode

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 22, 2016

Commit: 49c4488

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 22, 2016

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

49c4488Also defined this shortcut in GRSEvaluationPolynomialEncoder

@sagetrac-dlucas
Copy link
Mannequin

sagetrac-dlucas mannequin commented Feb 22, 2016

comment:3

E(message) now defaults to E.encode(message).
I added a few lines to the documentation of encode in encoder.py and in GRSEvaluationPolynomialEncoder to highlight this shortcut.

Setting this as needs_review.


New commits:

02c1f6dShortcut for method encode
49c4488Also defined this shortcut in GRSEvaluationPolynomialEncoder

@sagetrac-dlucas
Copy link
Mannequin

sagetrac-dlucas mannequin commented Feb 22, 2016

Author: David Lucas

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 25, 2016

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

de71938Same shortcut for C(m)

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 25, 2016

Changed commit from 49c4488 to de71938

@sagetrac-dlucas
Copy link
Mannequin

sagetrac-dlucas mannequin commented Feb 25, 2016

comment:6

I added the same mechanism with codes: let C be a code, and m an element from the message space of the encoder 'encoder'.

C(m, encoder_name='encoder') is now a shortcut to C.encode(m, encoder_name='encoder')

@sagetrac-dlucas
Copy link
Mannequin

sagetrac-dlucas mannequin commented Mar 18, 2016

comment:7

There's a doctest failing which indicates the test suite is broken for codes.
That's because in the test suite, there's an idempotency test for element construction.

It picks c = C.an_element() and then tries C(c)... Which of course fails in our case as __call__ has been redefined as encode - and the input of encode cannot be an element of C.

@sagetrac-dlucas
Copy link
Mannequin

sagetrac-dlucas mannequin commented Mar 18, 2016

comment:8

Because of the above, which is related to the way linear codes are set in the category framework, it seems impossible to do the proposed improvement.

Setting this to invalid/won't fix.

@sagetrac-dlucas sagetrac-dlucas mannequin removed this from the sage-7.1 milestone Mar 18, 2016
@johanrosenkilde

This comment has been minimized.

@johanrosenkilde
Copy link
Contributor Author

comment:9

This issue is not a show-stopper: instead of __call__ simply aliasing self.encode, it is a proper method which checks its input. If the input is a codeword, it returns that unchanged. If the input has the correct length of a message, return the encoded codeword, via self.encode. Otherwise, throw a ValueError.

Re-opening.

@johanrosenkilde johanrosenkilde changed the title E(message) for a linear code encoder E should encode message __call__(message) on a linear code or encoder should return a codeword. Mar 31, 2016
@johanrosenkilde johanrosenkilde added this to the sage-7.2 milestone Mar 31, 2016
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 31, 2016

Changed commit from de71938 to 872f5d7

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 31, 2016

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

4cd4b26Merge branch 'develop' into shortcut_to_encode
e8d7b58Merge branch 'develop' into shortcut_to_encode
872f5d7Updated `__call__` method with a proper behaviour

@sagetrac-dlucas
Copy link
Mannequin

sagetrac-dlucas mannequin commented Mar 31, 2016

comment:11

Done.
Except for the ValueError. If the input is not a codeword, it passes it to encode which does the check. As encode knows the message space of the encoder to use, it returns a nice error message which specifies the expected input (e.g. "The value to encode must be in Vector space of dimension 4 over Finite Field of size 2").

Maybe leave this review for Daniel as he wanted to do it? I just Cc'd him on this ticket.

@sagetrac-panda314
Copy link
Mannequin

sagetrac-panda314 mannequin commented Apr 1, 2016

comment:12

Hi, Maybe the check of whether the vector passed is a codeword or not can be done before passing it to the encode function. That way if a user means to pass a codeword rather than the message, and passed th wrong vector, he/she will be notified.

@johanrosenkilde
Copy link
Contributor Author

comment:13

David, didn't you accidentally put the Encoder-related code on the GRS encoder? It should be on the abstract class Encoder. Also, the check if m in self should be if m in self.code() on the Encoder. And your doc-test there doesn't test that method.

Replying to @sagetrac-panda314:

Hi, Maybe the check of whether the vector passed is a codeword or not can be done before passing it to the encode function. That way if a user means to pass a codeword rather than the message, and passed th wrong vector, he/she will be notified.

As far as I can see, the check in LinearCode does check for code membership before passing it to the encoder.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 1, 2016

Changed commit from 6b32cf0 to ce8d798

@sagetrac-dlucas
Copy link
Mannequin

sagetrac-dlucas mannequin commented Apr 1, 2016

comment:23

I vote for not defining Encoder.__call__ as an alias, then, but instead rely on dynamic dispatch to avoid the trap

Yes, it's a good idea. Done in the last commit.

@sagetrac-panda314
Copy link
Mannequin

sagetrac-panda314 mannequin commented Apr 1, 2016

comment:24

are the changes compiling? The copy of this branch is not compiling on my local pc (new to sage, am i missing something?). Here's the error:
InternalError: Internal compiler error: 'cysignals/signals.pxi' not found

Detailed message is,

Traceback (most recent call last):
  File "setup.py", line 590, in
    run_cythonize()
  File "setup.py", line 582, in run_cythonize
    'profile': profile,
  File "/home/panda/sage/local/lib/python2.7/site-packages/Cython-0.23.3-py2.7-linux-x86_64.egg/Cython/Build/Dependencies.py", line 758, in cythonize
    aliases=aliases)
  File "/home/panda/sage/local/lib/python2.7/site-packages/Cython-0.23.3-py2.7-linux-x86_64.egg/Cython/Build/Dependencies.py", line 664, in create_extension_list
    kwds = deps.distutils_info(file, aliases, base).values
  File "/home/panda/sage/local/lib/python2.7/site-packages/Cython-0.23.3-py2.7-linux-x86_64.egg/Cython/Build/Dependencies.py", line 564, in distutils_info
    return (self.transitive_merge(filename, self.distutils_info0, DistutilsInfo.merge)
  File "/home/panda/sage/local/lib/python2.7/site-packages/Cython-0.23.3-py2.7-linux-x86_64.egg/Cython/Build/Dependencies.py", line 574, in transitive_merge
    node, extract, merge, seen, {}, self.cimported_files)![0]
  File "/home/panda/sage/local/lib/python2.7/site-packages/Cython-0.23.3-py2.7-linux-x86_64.egg/Cython/Build/Dependencies.py", line 579, in transitive_merge_helper
    deps = extract(node)
  File "/home/panda/sage/local/lib/python2.7/site-packages/Cython-0.23.3-py2.7-linux-x86_64.egg/Cython/Build/Dependencies.py", line 555, in distutils_info0
    externs = self.cimports_and_externs(filename)![1]
  File "/home/panda/sage/local/lib/python2.7/site-packages/Cython-0.23.3-py2.7-linux-x86_64.egg/Cython/Utils.py", line 43, in wrapper
    res = cache[args] = f(self, *args)
  File "/home/panda/sage/local/lib/python2.7/site-packages/Cython-0.23.3-py2.7-linux-x86_64.egg/Cython/Build/Dependencies.py", line 465, in cimports_and_externs
    for include in self.included_files(filename):
  File "/home/panda/sage/local/lib/python2.7/site-packages/Cython-0.23.3-py2.7-linux-x86_64.egg/Cython/Utils.py", line 43, in wrapper
    res = cache[args] = f(self, *args)
  File "/home/panda/sage/local/lib/python2.7/site-packages/Cython-0.23.3-py2.7-linux-x86_64.egg/Cython/Build/Dependencies.py", line 448, in included_files
    include_path = self.context.find_include_file(include, None)
  File "/home/panda/sage/local/lib/python2.7/site-packages/Cython-0.23.3-py2.7-linux-x86_64.egg/Cython/Compiler/Main.py", line 274, in find_include_file
    error(pos, "'%s' not found" % filename)
  File "/home/panda/sage/local/lib/python2.7/site-packages/Cython-0.23.3-py2.7-linux-x86_64.egg/Cython/Compiler/Errors.py", line 177, in error
    raise InternalError(message)
InternalError: Internal compiler error: 'cysignals/signals.pxi' not found


Error building the Sage library


make: *** [sage] Error 1

@sagetrac-dlucas
Copy link
Mannequin

sagetrac-dlucas mannequin commented Apr 1, 2016

comment:25

It's compiling on my side. According to the error report you sent, a file is missing on your side.
I guess you'll have to download Sage again and recompile it...

@sagetrac-danielaugot
Copy link
Mannequin

sagetrac-danielaugot mannequin commented Aug 24, 2016

Changed branch from u/dlucas/shortcut_to_encode to u/danielaugot/shortcut_to_encode

@sagetrac-danielaugot
Copy link
Mannequin

sagetrac-danielaugot mannequin commented Aug 24, 2016

Changed commit from ce8d798 to 2fa1f17

@sagetrac-danielaugot
Copy link
Mannequin

sagetrac-danielaugot mannequin commented Aug 24, 2016

comment:27

I used the terminology "ambient space" in the doc string, to avoid being specific in details. Daniel


New commits:

2fa1f17Better doc string for __call__

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 24, 2016

Changed commit from 2fa1f17 to f25413e

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 24, 2016

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

f25413eMerge branch 'u/danielaugot/shortcut_to_encode' of git://trac.sagemath.org/sage into 20087/shortcut_to_encode

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 24, 2016

Changed commit from f25413e to 3ec380c

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 24, 2016

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

3ec380cfixed the docstring to match the ErrorValue

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 24, 2016

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

1fe5d5beasier to understand control structure using if-then-else

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 24, 2016

Changed commit from 3ec380c to 1fe5d5b

@johanrosenkilde
Copy link
Contributor Author

Changed branch from u/danielaugot/shortcut_to_encode to u/jsrn/shortcut_to_encode

@johanrosenkilde
Copy link
Contributor Author

comment:32

I mistakenly pushed another branch on top of this one. I've reverted it now -- sorry for the noise.

@ClementPernet
Copy link
Contributor

Changed keywords from none to rd3

@ClementPernet
Copy link
Contributor

comment:33

Doc string of the __call__ method of encoder (encoder.py:187) should test whether E(p) works not E.encode(p).

@ClementPernet
Copy link
Contributor

Reviewer: Clément Pernet

@ClementPernet
Copy link
Contributor

Changed branch from u/jsrn/shortcut_to_encode to u/cpernet/shortcut_to_encode

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 7, 2017

Changed commit from 1fe5d5b to 8ba64ee

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 7, 2017

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

8ba64eeadd missing doctest for C(word)

@ClementPernet
Copy link
Contributor

comment:36

It's all good, after the two minor docstring edits that I've pushed to the ticket. Positive review.

@vbraun
Copy link
Member

vbraun commented Feb 8, 2017

Changed branch from u/cpernet/shortcut_to_encode to 8ba64ee

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