#18376: New encoding structure for linear codes
-------------------------+-------------------------------------------------
Reporter: | Owner:
dlucas | Status: needs_work
Type: | Milestone: sage-6.8
enhancement | Resolution:
Priority: major | Merged in:
Component: | Reviewers:
coding theory | Work issues:
Keywords: | Commit:
Authors: David | 188b56f3367da3bcd1e5298dc012c663f5f08025
Lucas | Stopgaps:
Report Upstream: N/A |
Branch: |
u/dlucas/encoder |
Dependencies: |
-------------------------+-------------------------------------------------
Changes (by ncohen):
* status: needs_review => needs_work
Comment:
Helloooooo,
Here are my remarks/questions while I was looking for the first time at
your
code. Let's get this thing in so that you are not blocked for the more
mathematical parts.
- Vectorial space -> vector space?
- default implementation*s* of `encode()` and `unencode_nocheck()` methods
- call Encoder‘s `__init__()` in --> `Encoder.__init__` ?
- As Encoder is not designed to be implemented -> instanciated?
- Instead of typing the name of a method/class/function inside backticks,
you
should *link* to it, i.e. replace:
{{{Returns the code in which ``self.encode()`` has its output.}}}
with
{{{Returns the code in which :meth:`encode` has its output.}}}
You can then check by compiling the documentation with `--warn-links`
that
everything behaves as expected.
(happens in many places)
- Transforms an element of the message space into an element of the code
-->
into a codeword?
- You have a `unencode()` method with a `check` argument on one hand, and
a
`unencode_nocheck` method on the other hand. It makes me think that the
two
should be merged somehow, but I do not know exactly how they differ.
- What about rewriting the following message by creating a bibliography
entry
for codinglib?
{{{This function is taken from codinglib
(https://bitbucket.org/jsrn/codinglib/) and was written by Johan
Nielsen.}}}
It would become
{{{This function is taken from codinglib [Nielsen]_}}}
and link toward an entry where both the url and the authors are given.
- Documentation of `unencoder_matrix`:
{{{This is a helper function, for internal use only.}}}
If it is not meant for public use, can't we rename it to
`_unencoder_matrix`?
- `EncodingFailure` -> Given that most exceptions end with `Error`, what
about
`EncodingError`?
- Index of encoders -> It is a bit odd that you advertise `codes.encoders`
as
the way to get their list, while the entry that follows starts with
`linear_code.`
- `To import these names into the global namespace, use:` --> it sems that
the
code is *already* in the global namespace?...
- `encoder_default_name` -> `default_encoder`?
- `if(name in reg_enc.keys())` --> creates a temporary and useless list,
i.e. the list of keys. `if name in reg_enc` works.
- (honest question) what is the point of letting the users add encoders
with
this `add_encoder` method? If a user implements a new encoder, what does
he
earn by linking it inside of the instance instead of using it
independenly?
If there is such an advantage, could you illustrate it with an example
in the
function's docstring?
- When you accept `**kwargs` as input, please say that all additional
arguments
are accepted and forwarded to function <X>:
{{{def encode(self, word, name=None, **kwargs):}}}
- In the same `encode` function, I find the variable `name` a bit vague.
What
about `encoder_name` or `encoder`?
- The same happens in: `encoder()`, `generator_matrix`, `unencode`.
- Instead of `encoders_available`, what about simply `encoders`? I do not
see
the added value of `_available` there. Furthermore, what about returning
a
copy of the inner dictionary? Given that you can use a dictionary as you
would
use a list (`list(my_dict)`, `for x in my_dict`, `if x in my_dict`) why
wouldn't we return everything at once?
- `information_set` takes nothing as argument but calls `generator_matrix`
which
can depend on an encoder. Could you expose that flag and document it?
That's
the price of exposing in `LinearCode` the methods which are defined only
in
its encoder object.. It is a bit awkward `:-/`
- Really, these methods `encode` and `unencode` have nothing to do in
`LinearCode`. You should "accept your own design" and have all these
things be
available only in the `Encoder` object, otherwise there will be a crazy
amount
of copy/paste with those arguments.
- What about simplifying
{{{
The only purpose of this encoder is to set generic linear codes
into the new Encoder structure by providing a valid ``generator_matrix``
method.
}}}
As
{{{This is the default encoder of a generic linear code}}}?
- About:
{{{
Its ``generator_matrix`` method returns private field
``_generator_matrix``
of its associated code if any, else it calls the ``generator_matrix``
method
of the default encoder of the associated code.
}}}
Isn't that "over-documenting"? One can obain this kind of information by
reading the code (e.g. by adding ?? at the end of the function-->easy),
and it
is very unlikely that we will think of updating it if it ever changes.
- How is this
{{{
if hasattr(self.code(), "_generator_matrix"):
return self.code()._generator_matrix
else:
return self.code().generator_matrix()
}}}
better than {{{return self.code().generator_matrix()}}}?
- About:
{{{
class Encoder(SageObject):
...
This class provides:
- some methods for encoder objects
}}}
Err. Well, I could have guessed `:-P`
Why wouldn't you say "nothing there"? This kind of information can be
obtained
by tab completion or by browsing the html doc.
- If the following is meant for developers, I think we do not need it
(especially since the patches will either be written or reviewed by you
`:-P`). As for users, I can't see what they are expected to make of it.
Surely
they can call their classes as they like?
{{{
.. NOTE::
For consistency on encoders, please follow this convention on names
for subclasses:
for a new encoder named ``EncName``, for code family ``CodeFam``,
call it
``CodeFamEncNameEncoder``.
}}}
- If this `code` must be an instance of a specific class, could you make
it
explicit? It may be as simple as turning the second occurrence of 'code'
into
a Sphinx link toward a class.
{{{- ``code`` -- the associated code of ``self``}}}
- I do not understand why the `Encoder` class provides methods to
encode/decode
linear codes from the matrix, given that you cannot be sure that a
matrix is
available. Shouldn't this be in `LinearCodeGeneratorMatrixEncoder`?
- About having this 'name' parameter everywhere. What would be the problem
with
some `set_encoder` function which would define the instance's default
encoder,
and have all other functions call that? This could be defined from a
string
(using the list of available encoders) or by giving a class directly,
which
would make a public 'add_encoder' function useless, as users would have
an
easy way to plug their own encoder into the instance.
I am now in Nantes. It took me a Nice->Nantes flight to work on this
review. I
love this city. Plus the weather is good but not as hot as in the
south. Cooooooool.
Have fuuuuuuuuuuuun,
Nathann
--
Ticket URL: <http://trac.sagemath.org/ticket/18376#comment:15>
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.
