Opened 7 years ago

Last modified 7 years ago

#18099 closed enhancement

Prepare linear_code for inheritance — at Version 13

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.

LinearCode?'s need to be initialised with some magic incantations for them to work as modules and in the category framework. This needs to be called by all sub-classes as well, and could be achieved by creating an abstract linear code class.

Several private fields are also being set in the constructor which need to be set by all sub-classes. To avoid that subclasses need to know the name of these private fields (they should be accessed through public getters), we can instead set them using the abstract class constructor as well.

So, every sub-family of linear will only need to inherit from the abstract class.

Besides, a linear code gets his base_field using the base_ring() method coming from its category. Linear codes should have their own method to do that.

Change History (13)

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. ::


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.


comment:9 Changed 7 years ago by dlucas

  • Status changed from needs_info to needs_review

comment:10 Changed 7 years ago by ncohen

  • Status changed from needs_review to needs_work


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?


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.


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:// 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)
Note: See TracTickets for help on using tickets.