Opened 7 years ago

Last modified 7 years ago

#18099 closed enhancement

Prepare linear_code for inheritance — at Version 20

Reported by: dlucas Owned by:
Priority: major Milestone: sage-6.6
Component: coding theory Keywords: sd66
Cc: jsrn, ncohen Merged in:
Authors: David Lucas Reviewers:
Report Upstream: N/A Work issues:
Branch: u/dlucas/prepare_linear_code_for_inheritance (Commits, GitHub, GitLab) Commit: 8905552948a0896a46e587815cce539b6e60aae9
Dependencies: Stopgaps:

Status badges

Description (last modified by dlucas)

For now, every family of linear code (eg: Hamming code) is a method which returns a LinearCode object. It would be nice to change this: every family of code should be an object. Besides, every linear codes needs to know its generator matrix to be constructed. This is fine for linear codes without a specific algebraic structure, but not for sub-families of linear codes. For instance, it is possible to encode & decode words in Reed-Solomon codes without the help of a generator matrix. With regards to this, the user should be free to build the generator matrix for sub-families of code (which can be both a time- and memory-consuming operation). This is also true for the dimension of a code (which can be long to compute for some sub-families).

However, some parameters (like the length of the code, or its base field) are mandatory to every linear code, and subfamilies. They need to work fine with the category framework as well.

We then propose the following design:

implement an abstract class , AbstractLinearCode, which will initialize parameters used in every linear code, and make linear codes properly interact as modules in the category framework in its constructor. Besides, as all methods that were previously in linear codes need to work for all subfamilies of codes, we propose to relocate them as methods of AbtractLinearCode. With this design, every linear code and subfamily will only need to inherit from this abstract class to get all the generic methods and parameters initialized.

Besides, linear codes get their base_ring method from their category. For better consistency, we propose to implement a base_field() method which will be specific to linear codes.

Change History (20)

comment:1 Changed 7 years ago by dlucas

  • Branch set to u/dlucas/prepare_linear_code_for_inheritance

comment:2 Changed 7 years ago by dlucas

  • Authors set to David Lucas
  • Commit set to bf70359e1dd818e8dfc066ad2efa4ea03e06144d
  • Status changed from new to needs_review

New commits:

4d7f14dNew method: _init_linear_code
bf70359New method: base_field

comment:3 Changed 7 years ago by jsrn

  • Description modified (diff)

comment:4 Changed 7 years ago by ncohen

  • Cc ncohen added

comment:5 Changed 7 years ago by ncohen

Helloooooooooooooooooooo !

Several comments:

  • I do not understand why you created a specific function _init_linear_code instead of writing what you need in the __init__ function. In particular the names __init__ and _init_linear_code do not indicate how their aims are different.
    • If you want to change the name of the function, could it mention categories explicitly? (Or is there a reason not to?)
    • If you decide to remove this function, please move the doctests that it contains to the __init__ method.
  • You say in the ticket's description that you add a base_ring method while you implement base_field. What is the correct version?
  • I do not understand why you implement this function while saying that it is 'inherited'.
    • If it is inherited, why do you need to implement it?
    • If it is *not* inherited, then shouldn't we fix that instead of implementing the function?
  • We usually type '::' at the end of the line. Is there any reason why you write your code this way?
    We now create a member of our newly made class
    ::
    

Note that writing our newly made class:: will appear in the HTML as our newly made class: (only one ':'). If you do not want to see this terminal ':' then a space is sufficient, e.g.: This sentence ends here. ::

Nathann

Last edited 7 years ago by ncohen (previous) (diff)

comment:6 Changed 7 years ago by ncohen

  • Status changed from needs_review to needs_info

comment:7 Changed 7 years ago by git

  • Commit changed from bf70359e1dd818e8dfc066ad2efa4ea03e06144d to a425a89873d194bfdff1add267502a8f84ca6e08

Branch pushed to git repo; I updated commit sha1. New commits:

a425a89Changed structure of file: now contains an abstract class AbstractLinearCode for linear codes

comment:8 Changed 7 years ago by dlucas

Okay then!

I changed the structure of the file: we now have an AbstractLinearCode class which has all methods that was previously in LinearCode. LinearCode now inherits from this new abstract class (and gets all its former methods that way). I fixed the :: stuff too.

David

comment:9 Changed 7 years ago by dlucas

  • Status changed from needs_info to needs_review

comment:10 follow-up: Changed 7 years ago by ncohen

  • Status changed from needs_review to needs_work

Helloooooooooooooooooooooo,

Several comments:

  • (superficial) The way you moved code around makes the commit much more difficult to read than it should. It is a good thing to quickly read the commits before you push them, as usually the reviewer has to "reverse-engineer" what you want to do from the diff file. If you are forced to move code around, it is better to do so in a specific commit which *only* moves the code without changing anything in it.

As a reviewer, we have to make sure that all changes are correct, and as moving code around appears to us as 1) remove the code 2) add the code we have to re-read and check all of it, even if it was a no-brainer change on your side.

  • I changed your toy example a bit so that the matrix is given as argument. I hope that you will not mind.
  • While modifying minimum_distance() and gens() you interfered with its caching mechanism: answers which used to be cached are now forgotten. Could you fix that?
  • The doctest you wrote for the abstract class takes a lot of time, and we try to not go over a couple of seconds if we can avoid it (even for long ones). As this one does not test a very fundamental feature, could you make it a bit faster by changing the figures?

Thanks,

I added a small commit at public/18099. The usual procedure is that, as I reviewed your changes, you can be the reviewer of mine. In particular, feel free to discuss/oppose any of the changes.

Nathann

comment:11 Changed 7 years ago by jsrn

Also, remember to change the description of the ticket to reflect what it actually does now.

comment:12 Changed 7 years ago by git

  • Commit changed from a425a89873d194bfdff1add267502a8f84ca6e08 to 8905552948a0896a46e587815cce539b6e60aae9

Branch pushed to git repo; I updated commit sha1. New commits:

3ac3d64Merge branch 'u/dlucas/prepare_linear_code_for_inheritance' of git://trac.sagemath.org/sage into prepare_linear_code_for_inheritance
3fb0f56trac #18099: Merged with rc2
f7a8d03trac #18099: unimportant change in a doctest
6587ad8Merge with public branch
357c4f6Fixed too long doctest, put cached_method decorator over minimum_distance() and gens() methods
8905552Minor changes to documentation

comment:13 Changed 7 years ago by dlucas

  • Description modified (diff)

comment:14 in reply to: ↑ 10 Changed 7 years ago by dlucas

Replying to ncohen:

Helloooooooooooooooooooooo,

Hello!

Several comments:

  • (superficial) The way you moved code around makes the commit much more difficult to read than it should. It is a good thing to quickly read the commits before you push them, as usually the reviewer has to "reverse-engineer" what you want to do from the diff file. If you are forced to move code around, it is better to do so in a specific commit which *only* moves the code without changing anything in it.

As a reviewer, we have to make sure that all changes are correct, and as moving code around appears to us as 1) remove the code 2) add the code we have to re-read and check all of it, even if it was a no-brainer change on your side.

Got it! Thanks for the advice :)

  • I changed your toy example a bit so that the matrix is given as argument. I hope that you will not mind.

Not at all. I kept it in the new version of the code.

  • While modifying minimum_distance() and gens() you interfered with its caching mechanism: answers which used to be cached are now forgotten. Could you fix that?

Done. These methods are now cached.

  • The doctest you wrote for the abstract class takes a lot of time, and we try to not go over a couple of seconds if we can avoid it (even for long ones). As this one does not test a very fundamental feature, could you make it a bit faster by changing the figures?

Sure. Considering I picked some random methods to illustrate the inheritance mechanism with a dummy class, I just replaced it by something faster.

David

comment:15 Changed 7 years ago by dlucas

  • Status changed from needs_work to needs_review

comment:16 follow-up: Changed 7 years ago by vdelecroix

Hello,

I just had a quick look.

  1. What is the point of having two classes AbstractLinearCode and LinearCode. I would naturally name LinearCode the one from which I need to inherit from. Why not LinearCode and LinearCode_generic instead?
  1. Why the ambient vector space is not part of the input of AbstractLinearCode?
  1. What is the difference between base_ring provided by the Module and base_field?
  1. Coud you include in the docstring of the former AbstractLinearCode:
    • the set of attributes that are provided by the class
    • what should be done to implement a linar code when inheriting from it

In particular, some methods of AbstractLinearCode assume the presence of the attribute ._generator_matrix (like __cmp__).

  1. Could you remove
    + # sage: C.minimum_distance_upper_bound() # optional (net connection)
    + # 3
    + # sage: C.minimum_distance_why() # optional (net connection)
    + # Ub(7,4) = 3 follows by the Griesmer bound.
    
  1. In the docstring of AbstractLinearCode replace from this abstract method. with from this abstract class..

Vincent

comment:17 Changed 7 years ago by vdelecroix

  • Status changed from needs_review to needs_work

comment:18 in reply to: ↑ 16 Changed 7 years ago by ncohen

Hello,

  1. What is the point of having two classes AbstractLinearCode and LinearCode. I would naturally name LinearCode the one from which I need to inherit from. Why not LinearCode and LinearCode_generic instead?

I am not sure that I totally understand your question, but I may be able to help a bit.

The class which is currently named "LinearCode?" should (to me) be named "LinearCodeFromMatrix?" in the code [1]. It is meant to represent a linear code *defined* from a matrix. On the other hand, they will have some code which will *not* be defined from a matrix [2]. Thus, I thought that it made sense to have a class defining methods which apply to all kinds of codes in some astract class (I don't care much what the name of that class is).

Nathann

P.S.: As usual, when we discuss things "live", important information is left out from the ticket's comments.

[1] At user level, I don't see anything wrong with having it aliased as "LinearCode?", as it is the easiest way for users to define a code.

[2] Like Reed-Solomon codes. They apparently have better ways to compute things than to work on a matrix.

comment:19 Changed 7 years ago by dlucas

  • Description modified (diff)

comment:20 Changed 7 years ago by dlucas

  • Description modified (diff)
Note: See TracTickets for help on using tickets.