Changes between Initial Version and Version 1 of Ticket #18099, comment 25


Ignore:
Timestamp:
04/09/15 08:58:32 (7 years ago)
Author:
jsrn
Comment:

Legend:

Unmodified
Added
Removed
Modified
  • Ticket #18099, comment 25

    initial v1  
    1 Replying to [comment:24 vdelecroixt
    2 > Replying to [comment:21 dlucas]:
    3 > > Hello,
    4 > >
    5 > > Regarding the naming problem
    6 > > [...]
    7 >
     1Hi Vincent
     2
     3It'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.
     4
    85> 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.
    9 >
    10 > > >Why the ambient vector space is not part of the input of AbstractLinearCode?
    11 > >
    12 > > It is actually implied by the parameters:
    13 > > [...]
    14 >
    15 > All right. That is at least clearer to me. Thanks for your explanation.
    16 >
    17 > > >What is the difference between base_ring provided by the Module and base_field?
    18 > >
    19 > > 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!
    20 >
    21 > For vector space, you have
    22 > {{{
    23 > def base_field(self):
    24 >     ...
    25 >     return self.base_ring()
    26 > }}}
    27 > (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`.
    28 >
    29 > 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.
    30 >
    31 > > >Could you remove
    32 > > [...]
    33 > > 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.
    34 >
    35 > All right
    36 >
    37 >
    38 > Next:
    39 >
     6Ok, we'll go with the abstract class then.
     7
     8> For vector space, you have ...
     9This implementation of `base_field` is indeed better, and we can kill `_base_field`.
     10
     11> 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`...
    4012> - It is very wrong to set
    4113> {{{
    4214> Element = type(facade_for).an_element()
    4315> }}}
    44 >   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.
    45 >
     16> ...
     17We didn't write this facade code; it's from #16644. We're not sufficiently
     18familiar with the category framework to judge whether it's sensible or not. If
     19it is not good as you say, it should of course be fixed, but I suggest doing
     20that in another ticket since it implies various other cleanup (e.g.
     21`__contains__` and `ambient_space` as you mention). I therefore suggest adding
     22`base_field` with the simple implementation above -- to fix the interface in
     23this ticket - and then create a new ticket to fix the category stuff (which I
     24hope you can further help us with).
     25
    4626> - This badly fails
    4727> {{{
     
    4929> <repr(<sage.coding.linear_code.AbstractLinearCode_with_category at 0x7f6867c61890>) failed: AttributeError: 'AbstractLinearCode_with_category' object has no attribute '_generator_matrix'>
    5030> }}}
     31
     32It's supposed to fail: it's an abstract class, so you shouldn't instantiate it.
     33
    5134>   One way is to provide a generic is just to move the `_repr_` in `LinearCode` to `AbstractLinearCode`.
    52 >
     35The usual `_repr_` should be in `LinearCode`, and I don't see a reason for defining `AbstractLinearCode._repr_`. I guess it could possibly be flagged with `@abstract_method` but that's largely a matter of taste. In this instance, I don't find it integral to the functionality of sub-classes that they have a `_repr_`, and `@abstract_method` is intended for flagging only essential-to-override methods.
     36
    5337> - the method `AbstractLinearCode.generator_matrix` is implemented as `return self._generator_matrix`. Why not
    5438> {{{
     
    5640>     raise NotImplementedError("This method must be set in subclasses")
    5741> }}}
    58 >   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`.
     42
     43The 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.
     44
     45We made this mistake in the current code because we're introducing a generic way to handle encoders in a ticket coming up. Encoders are linked to generator matrices, and this framework will override `AbstractLinearCode.generator_matrix` with a method which asks the "default" encoder for the generator matrix. In this sense, `AbstractLinearCode.generator_matrix` becomes a convenience function, and more or less no sub-class will ever need to override it.
     46
    5947>
    6048> - In the docstring of the class, you forgot to mention that the method `generator_matrix` must be implemented in subclasses.
     49Subclasses 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.)
     50
    6151>
    6252> - 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).
    63 >
    64 > - the method `zero` is badly implemented. Instead, just do (without caching)
    65 > {{{
    66 > def zero(self):
    67 >     return self.ambient_space().zero()
    68 > }}}
    69 >   Or if you really care about the non-facade cases
    70 > {{{
    71 > def zero(self):
    72 >     if self.facade_for():
    73 >         return self(0)
    74 >     else:
    75 >         return self.ambient_space().zero()
    76 > }}}
    77 >
     53Ok. 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.
     54
     55> - the method `zero` is badly implemented...
     56
     57Lot's of stuff is badly implemented (and we know). But it's not the job of this ticket to fix everything in `linear_code.py`.
     58
    7859> - 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!!
    79 >
     60You're right, `@cached_method` should be removed.
     61
    8062> - In the constructor, don't you want to check that the argument `base_field` provided is actually a finite field? An easy way is
    8163> {{{
     
    8466> }}}
    8567>   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`).
    86 >
    87 > Enough for now...
    88 >
    89 > Vincent
     68
     69Note that such a check wasn't there before either (matrices can be over rings
     70too). But the check makes sense so we can add it. Probably `length` should be
     71cast to Python `int`, and then cast to `Integer` in the `def length()` method.