#18376: New encoding structure for linear codes
-------------------------+-------------------------------------------------
Reporter: | Owner:
dlucas | Status: needs_review
Type: | Milestone: sage-6.9
enhancement | Resolution:
Priority: major | Merged in:
Component: | Reviewers:
coding theory | Work issues:
Keywords: | Commit:
Authors: David | d132d3cfde4cb174cd5a03e3d6d56d51f5752cc7
Lucas | Stopgaps:
Report Upstream: N/A |
Branch: |
u/dlucas/encoder |
Dependencies: |
-------------------------+-------------------------------------------------
Comment (by jsrn):
Ok, some comments, small and slightly less small:
1. Docstring of `class Encoder` says "... needed to describe properly the
code defined in the subclass". "code" should be "encoder". But probably
that sentence confuses more than it helps in any case :-)
1. Same docstring, why is "Then, if the message space..." and the
following paragraph not part of the list?
1. The paragraph "Equality methods ..." is a bit strange. Perhaps
By default, comparison of `Encoder`s (the methods``__eq__`` and
``__ne__``)
are by memory reference: If you build the same encoder twice, they
will be
different. If you need something more clever, override ``__eq__`` and
``__ne__`` in you subclass.
This paragraph should also be in the list. The last paragraph on
representation should also be in the list.
1. Same docstring. If message space is not `F^k`, you also need to
override `message_space`.
1. Docstring of `Encoder.encode`, `INPUT block "the code" should be
`self`.
`OUTPUT` block. `self` should be `self.code`.
1. `Encoder.encode`. The `ValueError` should not refer to "vector", I
think, to set the precedent of this error message for other encoders. Say
`The value to encode` instead, for instance. Also remove the `a` just
before `M` in that error.
1. Docstring of `Encoder.unencode` could perhaps be more precise. Perhaps
instead "The inverse of `encode`: return the message corresponding to the
codeword ``c``." Also the `INPUT` block seem to have been wrongly
copy/pasted from a time when this was in `LinearCode`. Also, last sentence
in describing ``nocheck`` could perhaps more enlightening be "You might
set this to ``True`` to disable the check for saving computation. Note
that if ``c`` is not in ``self`` and ``nocheck = True``, then the output
of `unencode` is not defined (except that it will be in the message space
of `self`).
1. `Encoder.unencode`. You don't need all the `else` stuff: remove the
first else branch completely, and always run the second `return
self.unencode_nocheck(c)`.
1. `Encoder.unencode_nocheck`. Perhaps the information set corresponding
to `_unencoder_matrix` should be saved together with that matrix.
Currently, the code -- rather brittly -- depends on
`self.code.information_set` always returning the same result. Which is not
part of that method's docstring (even though you now added
`@cached_method` on its default implementation, which should still be
there).
1. `Encoder.unencode_nocheck`, docstring. I like that you added an example
for a vector not in `C`. But perhaps you could reformulate that sentence
to say something like "Taking a vector that does not belong to C will not
raise an error, but probably just give a non-sensical result".
1. `Encoder.code` docstring. Perhaps just "Returns the code for this
`Encoder`"? Also, in the Example, last line could be `E.code() == C`
instead.
1. `Encoder.encode` docstring: after reading the docstring of
`Encoder.message_space`, I think that perhaps a reference to the possible
partial function nature of `Encoder.encode` should be given there towards
the end, as an information to future `Encoder` developers? Perhaps say
what error should be raised in that case.
1. Perhaps the `encoders` import statement in `coding/codes_catalog`
should also be lazy? I know that `codes_catalog` is already lazy, but
`encoders` will usually not be used, even if you use `codes_catalog`.
1. Index of encoders, docstring could be generated with the new auto-table
functions Nathann made.
1. `class AbstractLinearCode` docstring: I don't understand the point of
`This class provides: ...`. Why is ``length`` there? There is a private
property `_length` and a method `length()`. But for instance,
`dimension()` is not mentioned? Why do you list `default_encoder_name` and
`_registered_encoders` there? Perhaps
1. `AbstractLinearCode`: the error thrown when giving an unknown encoder
could perhaps hint at *how* to add this default encoder.
1. `AbstractLinearCode` docline "fill the dictionary..." should add `.py`
to the name of `__init__`.
1. `AbstractLinearCode` docstring on `__cmp__` and `__eq__` could perhaps
be clearer. Something like "AbstractLinearCode has generic implementations
of the comparison methods `__cmp__` and `__eq__` which use the generator
matrix and are quite slow. In subclasses you are encouraged to override
these functions." Is `__cmp__` and `__eq__` the exhaustive list of
comparison functions?
1. `AbstractLinearCode.add_encoder`. Perhaps the doc should clarify that
the added encoder is only added to `self` and not any member of the class.
As well as point the reader to how to do that instead.
1. `AbstractLinearCode.add_encoder`. I don't like that all newly created
codes copy the dictionary of registered encoders, when almost always,
`add_encoder` is not going to be called on a created code. Instead, I
think that in `add_encoder` you can add a check to see if
`self._registered_encoders is self.__class__._registered_encoders` or
something. If this is the case, *then* you make a new copy of
`self._registered_encoders` and add the new encoder to it. Otherwise,
`add_encoder` must have been already called on this code and you shouldn't
make a new copy of course.
1. `AbstractLinearCode.encode` doc is missing punctuation many places.
Also in `encoder`. Maybe also elsewhere.
1. `AbstractLinearCode.encode` docstring says "the message space". Should
be "a message space". It should say prominently in the doc of both
`encode` and `encoder` that the default encoder always has `F^k` as
message space. Come to think of it, that should also be stated very
clearly in the doc of `class AbstractLinearCode`.
1. `AbstractLinearCode.encoder` docstring refers to `name` but its
`encoder_name`. The doc should cleary describe that the encoder is cached
(right now it's only indirectly referred to in the last pargraph). I think
there should be a test of this.
1. `AbstractLinearCode.encoder` the line `return
self.encoder(encoder_name, **kwargs)` can simply be removed.
1. `AbstractLinearCode.encoders_available` the variable `reg_enc` is
superfluous and not even used, two lines below. Perhaps the argument
`values` could be renamed to `classes`.
1. `AbstractLinearCode.encode` and `.generator_matrix` and `.unencode` doc
of `kwargs` and `LinearCode.generator_matrix`. Would it be more helpful to
write "all additional arguments are forwarded to the construction of the
encoder that is used."
1. `AbstractLinearCode.unencode` doc could be made more precise, like the
`Encoder.unencode` doc. The doc of input `c` should just say that `c` is a
codeword of `C`. `nocheck` should also be clarified like in `Encoder`. The
OUTPUT block is uninformative and plain wrong (if `encoder_name` is set to
something).
1. `LinearCode.generator_matrix`. Why does this check that
`_generator_matrix` exists? That is always the case? More importantly, if
`encoder_name` is not `None` or `GeneratorMatrix`, then this function
should not return `_generator_matrix` but `<that
encoder>.generator_matrix()`. That could be more elegantly handled by a
call to `super`'s `generator_matrix` function.
1. `LinearCodeGeneratorMatrixEncoder._repr_` and `._latex_` should include
the word "the" just before the code, I think.
Phew, I think that's it :-)
Best,
Johan
--
Ticket URL: <http://trac.sagemath.org/ticket/18376#comment:30>
Sage <http://www.sagemath.org>
Sage: Creating a Viable Open Source Alternative to Magma, Maple, Mathematica,
and MATLAB
--
You received this message because you are subscribed to the Google Groups
"sage-trac" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To post to this group, send email to [email protected].
Visit this group at http://groups.google.com/group/sage-trac.
For more options, visit https://groups.google.com/d/optout.
