#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):
Replying to [comment:21 dlucas]:
> Hello,
>
> Regarding the naming problem
> [...]
If you look around, there are very few `AbstractXYZ`. On the other hand, I
do not have very strong feeling about that, so I let you decide.
> >Why the ambient vector space is not part of the input of
AbstractLinearCode?
>
> It is actually implied by the parameters:
> [...]
All right. That is at least clearer to me. Thanks for your explanation.
> >What is the difference between base_ring provided by the Module and
base_field?
>
> We noticed that some classes in Sage (like for instance `VectorSpace`)
have both `base_ring` and `base_field` methods implemented, the former
coming from the category framework and the latter specifically made for
the class, both returning the same result. We chose to follow that because
we thought it was consistent with Sage to do so. If it's not, I can remove
the `base_field` method!
For vector space, you have
{{{
def base_field(self):
...
return self.base_ring()
}}}
(see line 5144 of `sage/modules/free_module.py`). Why would you dupliacte
the attribute in your class `LinearCode`? Note that I have nothing against
a '''method''' `base_field` but you should get rid of the '''attribute'''
`_base_field`.
Actually, one thing that I would find more natural is to inherit from
`FreeModule_submodule_field` (from `sage.modules.free_module`) instead of
simply `Module`. You will get for free many very useful methods (like
`ambient_space`, a much better implementation of `__contains__`, etc). The
drawback is that you need the generator matrix in the constructor. But I
do not see why this would be so bad.
> >Could you remove
> [...]
> If you agree, we would prefer to focus on integrated our new
functionnalities into Sage for now, and once it is done clean up all the
coding library based on what we noticed while working on the existing
code, and on the new input we might get.
All right
Next:
- It is very wrong to set
{{{
Element = type(facade_for).an_element()
}}}
The thing is that in the category mechanism, `Element` is just here to
produce the attribute `_element_class` which is then used as the class for
elements. Except in the case where the class is an extension class (i.e. a
Cython class) those two classes are different. Moreover, if at some point
you implement a `LinearCode` which is not a facade, you will need to set
an attribute `Element` to it. In the current state, this attribute would
be overriden in the `__init__` by mentioned line.
- 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'>
}}}
One way is to provide a generic is just to move the `_repr_` in
`LinearCode` to `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")
}}}
That way, the programmer knows where there is something wrong when he
implements a new code. Another way to do is to use the decorator
`@abstract_method` from `sage.misc.abstract_method`.
- In the docstring of the class, you forgot to mention that the method
`generator_matrix` must be implemented in subclasses.
- 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).
- the method `zero` is badly implemented. Instead, just do (without
caching)
{{{
def zero(self):
return self.ambient_space().zero()
}}}
Or if you really care about the non-facade cases
{{{
def zero(self):
if self.facade_for():
return self(0)
else:
return self.ambient_space().zero()
}}}
- Why did you set `gens` as a `cached_method`? Why do you check for a
`_gen` attribute there? If you have an attribute `_gen` at some point the
`cached_method` feature will just duplicate the fact that you already have
an attribute that takes care of caching!!
- In the constructor, don't you want to check that the argument
`base_field` provided is actually a finite field? An easy way is
{{{
if not base_field in Fields().Finite():
raise ValueError("base_field (={}) must be a finite
field".format(base_field))
}}}
the argument `length` should also be checked and converted to the type
you want to use (I guess either a Python `int` or a Sage `Integer`).
Enough for now...
Vincent
--
Ticket URL: <http://trac.sagemath.org/ticket/18099#comment:24>
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.
