#18099: Prepare linear_code for inheritance
-------------------------------------+-------------------------------------
Reporter: dlucas | Owner:
Type: enhancement | Status: needs_work
Priority: major | Milestone: sage-6.6
Component: coding theory | Resolution:
Keywords: sd66 | Merged in:
Authors: David Lucas | Reviewers:
Report Upstream: N/A | Work issues:
Branch: | Commit:
u/dlucas/prepare_linear_code_for_inheritance|
1652d7c6f5dfc558d3f0b56882e2f51b7d82decb
Dependencies: | Stopgaps:
-------------------------------------+-------------------------------------
Comment (by vdelecroix):
Hello Johan, hello David,
Replying to [comment:25 jsrn]:
> It's awesome that you're being so thorough and giving feedback, but I
think you're asking for too many things in this ticket. There's many
things wrong with the current code, but for this ticket, we're not trying
to fix it all. I'll reply to your comments below.
No problem. I am just saying what I found wrong while reading the code.
You are free to say "this is not for this ticket". But keep my remarks
under your pillow for the next tickets.
You somehow ignore my most important request: inheritance from
`FreeModule_submodule_field`. That would avoid implementing plenty of
useless stuff like `__contains__`, `cardinality`, `__iter__` and so on.
> > - It is very wrong to set
> > {{{
> > Element = type(facade_for).an_element()
> > }}}
> > ...
> [...]
What happens if you just get rid of that line? Category is a driven by
error experimentation ;-)
> > - This badly fails
> > {{{
> > sage: sage.coding.linear_code.AbstractLinearCode(GF(3),2)
> > <repr(<sage.coding.linear_code.AbstractLinearCode_with_category at
0x7f6867c61890>) failed: AttributeError:
'AbstractLinearCode_with_category' object has no attribute
'_generator_matrix'>
> > }}}
>
> It's supposed to fail: it's an abstract class, so you shouldn't
instantiate it.
On the other hand the implementation uses the attribute
`_generator_matrix`. Perhaps, you could just remove this `_repr_` in
`AbstractLinearCode`.
> > - the method `AbstractLinearCode.generator_matrix` is implemented as
`return self._generator_matrix`. Why not
> > {{{
> > def generator_matrix(self):
> > raise NotImplementedError("This method must be set in subclasses")
> > }}}
>
> The current implementation should indeed be in `LinearCode`. In
`AbstractLinearCode` it should raise a `NotImplementedError`, though not
with the message you suggest: mathematically, every linear code has a
generator matrix (it has many), but from a programming point of view, one
should not be forced to implement it for every code class.
> > - In the docstring of the class, you forgot to mention that the method
`generator_matrix` must be implemented in subclasses.
> Subclasses don't need to. (sure, lot's of functions in
`AbstractLinearCode` will throw a `NotImplementedError` with the above
default implementation of `generator_matrix`, but that's ok.)
The thing is that many methods in `AbstractLinearCode` will just fail if
you do not implement the method `generator_matrix` (like `gens`,
`__iter__`, `information_set`, `is_permutation_automorphism`,
`is_permutation_equivalent`, ...). I agree that there are many generating
matrices, but in order to define properly your subspace you need to tell
Sage at least one. Perhaps not through the method `generating_matrix`
though. This is of course related to the inheritance from
`FreeModule_submodule_field`.
> We made this mistake in the current code because we're introducing a
generic way to handle encoders in a ticket coming up. [...]
I am not familiar with encoders (nor code theory by the way ;-).
> > - the `NOTE::` section in the docstring should be a `.. NOTE::`
section. Moreover, the lines are too large here (>110 characters and
should be around 80).
> Ok. On a side-note, is the 80-character wrap really enforced everywhere?
IMHO, it is archaic and very impractical to enforce, especially for doc-
tests.
It is not strictly requested. But the two dots before `NOTE` are.
That is great work to clean all of this... a bit tedious but definitely
needed!
Best
Vincent
--
Ticket URL: <http://trac.sagemath.org/ticket/18099#comment:26>
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.
