Fix LinearCode.wtdist_gap method

5b3bd7e6-c3f3-4830-a555-991f5e6beec0 commented 8 years ago

There are three main issues with the wtdist_gap method namely: 1) Name of the method 2) Poor documentation 3) It should be a private function

CC: @sagetrac-dlucas @johanrosenkilde

Component: coding theory

Author: Arpit Merchant

Branch/Commit: 191d1c4

Reviewer: David Lucas

Issue created by migration from https://trac.sagemath.org/ticket/20565

5b3bd7e6-c3f3-4830-a555-991f5e6beec0 commented 8 years ago

Attachment: wtdist_gap.txt

Notes on the issues with the method and proposed corrections.

5b3bd7e6-c3f3-4830-a555-991f5e6beec0 commented 8 years ago

Branch: u/arpitdm/fix_wtdist_gap_method

5b3bd7e6-c3f3-4830-a555-991f5e6beec0 commented 8 years ago

New commits:

3c62c14 improved name and documentation of the wtdist_gap method. made this a private method.

5b3bd7e6-c3f3-4830-a555-991f5e6beec0 commented 8 years ago

Commit: 3c62c14

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

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

161a268 placed method inside AbstractLinearCode class. made it private by using a single leading underscore. improved documentation to show how to use the function. updated description of Gstr in documentation.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

Changed commit from 3c62c14 to 161a268

1861b8a9-77f0-4f35-8431-8514a75b40d1 commented 8 years ago

comment:7

Hello Arpit,

A few remarks:

in Sage, you cannot delete/change the signature of a method silently, you have to support the old method for a year. To do that, you need to use deprecation warnings to notify users about the new functionality (and warn them the old one will be deleted soon). You can use the following line: wtdist_gap = deprecated_function_alias(20565, _weight_distribution). Then, call the old wtdist_gap and you should see a warning.
I'm not sure the weight distribution should be a private method: it's an interesting "property" to compute, and I'm sure some users expect to see it here (maybe Johan can confirm what I'm saying). In any case, I have a specific method weight_distribution in grs.py (as you have a faster algorithm to compute it on MDS codes) and it's not private. So, for consistency, it should be private everywhere or nowhere (I vote for nowhere).
Also, I think we can completely remove the input of this method: as you're making it a method of AbstractLinearCode, it's no longer needed to pass n and F (as self.length() and self.base_ring() will recover n and F) and I think having to pass the generator matrix in it's GAP string format is very annoying... One should just call C.weight_distribution() and get the list. It's possible to get the GAP format of a matrix by typing M._gap_().

Otherwise, I agree with other changes, the documentation and the new names.

Best,

David

johanrosenkilde commented 8 years ago

comment:8

I'm not sure the weight distribution should be a private method: it's an interesting "property" to compute, and I'm sure some users expect to see it here...

The function that Arpit has been working on is a helper-function for AbstractLinearCode.spectrum which has the alias AbstractLinearCode.weight_distribution. That method has the signature you ask for. The function Arpit has modified is just a helper for that.

Perhaps it should therefore be named AbstractLinearCode._spectrum_from_gap. Its signature could still be changed to not take any arguments if one just changes how it is called in spectrum. That would make sense.

Best, Johan

5b3bd7e6-c3f3-4830-a555-991f5e6beec0 commented 8 years ago

comment:9

Deprecation warnings: Understood. I'm adding the line now.
Name and Private: AbstractLinearCode.spectrum computes the spectrum using three algorithms "binary", "gap" and "leon" each of which is implemented differently. "binary" imports a function from another file and then calls the "weight_dist" function from it while "leon" is implemented within spectrum. I think the entire method needs to be refactored so that the code is more modular. And so perhaps, the name weight_distribution is a misnomer. Like Johan suggests, it should be titled _spectrum_from_gap (and so on for the other two as well).
Input to the method: Agreed. I'll make changes accordingly.

1861b8a9-77f0-4f35-8431-8514a75b40d1 commented 8 years ago

comment:10

Replying to @johanrosenkilde:

I'm not sure the weight distribution should be a private method: it's an interesting "property" to compute, and I'm sure some users expect to see it here...

The function that Arpit has been working on is a helper-function for AbstractLinearCode.spectrum which has the alias AbstractLinearCode.weight_distribution. That method has the signature you ask for. The function Arpit has modified is just a helper for that.

Perhaps it should therefore be named AbstractLinearCode._spectrum_from_gap. Its signature could still be changed to not take any arguments if one just changes how it is called in spectrum. That would make sense.

Best, Johan

Yes, you're right, please ignore my remarks on this!

David

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

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

1114267 added deprecation warning for the changed method name. removed input parameters since they are directly accessible. changed name to _spectrum_from_gap().

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

Changed commit from 161a268 to 1114267

1861b8a9-77f0-4f35-8431-8514a75b40d1 commented 8 years ago

comment:12

Hello,

There's still a few problems:

You deleted some lines from spectrum, especially the ones defining the local variables (like F = self.base_ring())... Which results in several failures in this method (l. 3306, for instance, returns global name F not known) because it tries to use variables that are no longer defined.
(this is quite my fault), the deprecation warning does not work. As you put wtdist_gap = deprecated_function_alias(20565, _spectrum_from_gap) in AbstractLinearCode, it does not respect the old signature anymore. With the deprecation mechanism working as expected, one should be able to call

sage: Gstr = 'Z(2)*[[1,1,1,0,0,0,0], [1,0,0,1,1,0,0], [0,1,0,1,0,1,0], [1,1,0,1,0,0,1]]'
sage: F = GF(2)
sage: sage.coding.linear_code.wtdist_gap(Gstr, 7, F)

get a warning and the expected result. Plus, as I suggested you to remove the input from _spectrum_from_gap, if one calls C.wtdist_gap(Gstr, 7, F) because C._spectrum_from_gap(Gstr, 7, F) does not exist...

So, I propose to reinstate wtdist_gap where it was and as it was, but to change its code: now it will send a deprecation warning and call _spectrum_from_gap. It should look like this:

 def wtdist_gap(Gmat, n, F):
    from sage.misc.superseded import deprecation
    deprecation(20565, "wtdist_gap is now deprecated. Please use AbstractLinearCode._spectrum_from_gap instead.")
    #create a linear code C
    return C._spectrum_from_gap()

It's kind of an idiotic behaviour but it will work.

Best,

David

5b3bd7e6-c3f3-4830-a555-991f5e6beec0 commented 8 years ago

comment:13

I've added back the definitions of the local variables such as F = self.base_ring().

The reason to put back wtdist_gap as it was, is to allow for users to get accustomed to the changes, right? That's why we need all the redirects.

Do we want that spectrum also call wtdist_gap() which then calls _spectrum_from_gap()? Or can spectrum directly call the latter?
The original wtdist_gap took the generator matrix in a gap string format. Is it possible to reconvert this representation into a regular generator matrix from which a linear code can be constructed?

1861b8a9-77f0-4f35-8431-8514a75b40d1 commented 8 years ago

comment:14

I've added back the definitions of the local variables such as F = self.base_ring().

Ok, good!

The reason to put back wtdist_gap as it was, is to allow for users to get accustomed to the changes, right? That's why we need all the redirects.

Exactly. In Sage, whenever you change something, you have to be retrocompatible with the old feature for a year.

Do we want that spectrum also call wtdist_gap() which then calls _spectrum_from_gap()? Or can spectrum directly call the latter?

spectrum can directly call _spectrum_from_gap. With all these deprecation warnings, the general rule is to allow the user to access the old feature for a year. So, as long as you still provide the old method/signature/output, replacing the old stuff everywhere else is fine.

The original wtdist_gap took the generator matrix in a gap string format. Is it possible to reconvert this representation into a regular generator matrix from which a linear code can be constructed?

Yup, for instance:

sage: M = matrix(GF(7), [[1,2,3], [4,5,6]])
sage: Mg = M._gap_()
sage: Ms = Mg._matrix_(GF(7))
sage: Ms == M
True

5b3bd7e6-c3f3-4830-a555-991f5e6beec0 commented 8 years ago

comment:15

One more thing. We need to have the original wtdist_gap documentation also. The doctest there fails with the following message:

File "src/sage/coding/linear_code.py", line 315, in sage.coding.linear_code.wtdist_gap
Failed example:
    sage.coding.linear_code.wtdist_gap(Gstr, 7, F)
Expected:
    DeprecationWarning: wtdist_gap is now deprecated. Please use AbstractLinearCode._spectrum_from_gap instead.
    See http://trac.sagemath.org/20565 for details.
    [1, 0, 0, 7, 7, 0, 0, 1]
Got:
    doctest:1: DeprecationWarning: wtdist_gap is now deprecated. Please use AbstractLinearCode._spectrum_from_gap instead.
    See http://trac.sagemath.org/20565 for details.
    [1, 0, 0, 7, 7, 0, 0, 1]

What is the correct way to add the deprecation warning to the doctest?

When I run the wtdist_gap method, it gives a bunch of other deprecation warnings. Are these fine or do I need to change anything here?

sage: sage.coding.linear_code.wtdist_gap(Gstr, 7, F)
/home/arpit/Documents/GSOC_16/sage-7.1/src/bin/sage-ipython:1: DeprecationWarning: wtdist_gap is now deprecated. Please use AbstractLinearCode._spectrum_from_gap instead.
See http://trac.sagemath.org/20565 for details.
  #!/usr/bin/env python
/home/arpit/Documents/GSOC_16/sage-7.1/local/lib/python2.7/site-packages/IPython/core/formatters.py:92: DeprecationWarning: DisplayFormatter._ipython_display_formatter_default is deprecated: use @default decorator instead.
  def _ipython_display_formatter_default(self):
/home/arpit/Documents/GSOC_16/sage-7.1/local/lib/python2.7/site-packages/IPython/core/formatters.py:98: DeprecationWarning: DisplayFormatter._formatters_default is deprecated: use @default decorator instead.
  def _formatters_default(self):
/home/arpit/Documents/GSOC_16/sage-7.1/local/lib/python2.7/site-packages/IPython/core/formatters.py:677: DeprecationWarning: PlainTextFormatter._deferred_printers_default is deprecated: use @default decorator instead.
  def _deferred_printers_default(self):
/home/arpit/Documents/GSOC_16/sage-7.1/local/lib/python2.7/site-packages/IPython/core/formatters.py:669: DeprecationWarning: PlainTextFormatter._singleton_printers_default is deprecated: use @default decorator instead.
  def _singleton_printers_default(self):
/home/arpit/Documents/GSOC_16/sage-7.1/local/lib/python2.7/site-packages/IPython/core/formatters.py:672: DeprecationWarning: PlainTextFormatter._type_printers_default is deprecated: use @default decorator instead.
  def _type_printers_default(self):
/home/arpit/Documents/GSOC_16/sage-7.1/local/lib/python2.7/site-packages/IPython/core/formatters.py:669: DeprecationWarning: PlainTextFormatter._singleton_printers_default is deprecated: use @default decorator instead.
  def _singleton_printers_default(self):
/home/arpit/Documents/GSOC_16/sage-7.1/local/lib/python2.7/site-packages/IPython/core/formatters.py:672: DeprecationWarning: PlainTextFormatter._type_printers_default is deprecated: use @default decorator instead.
  def _type_printers_default(self):
/home/arpit/Documents/GSOC_16/sage-7.1/local/lib/python2.7/site-packages/IPython/core/formatters.py:677: DeprecationWarning: PlainTextFormatter._deferred_printers_default is deprecated: use @default decorator instead.
  def _deferred_printers_default(self):
[1, 0, 0, 7, 7, 0, 0, 1]

1861b8a9-77f0-4f35-8431-8514a75b40d1 commented 8 years ago

comment:16

One more thing. We need to have the original wtdist_gap documentation also. The doctest there fails with the following message

Usually, I just delete the old documentation and leave only the code of the old method. See for instance what I did with ReedSolomonCode in code_constructions.py.

My argument is the following: as it's an old feature we keep only for retrocompatibility purposes, I don't see the point of keeping its documentation. Indeed, users which have old code using the deprecated feature already know how the old method works and are using it correctly, so they don't need the documentation. On any other case, people are not supposed to use the old feature, and maintaining proper documentation for an old feature might confuse them and lead them to use it - while we don't want them to do that!

When I run the wtdist_gap method, it gives a bunch of other deprecation warnings. Are these fine or do I need to change anything here?

That's fine, don't worry!

David

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

Changed commit from 1114267 to 10d7ca7

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

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

10d7ca7 reinstated the input, signatures and output of the now-deprecated wtdist_gap method.

1861b8a9-77f0-4f35-8431-8514a75b40d1 commented 8 years ago

comment:18

Hello,

One last tweak: l. 530 in linear_code.py, references := ... became :3references := ... I assume it's a typo. Can you fix it please? Otherwise, tests passes and I agree with your changes, so as soon as this typo is fix, I'll give this ticket a positive_review.

Well done!

David

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

Changed commit from 10d7ca7 to 191d1c4

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

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

191d1c4 fixed small typo that had inadvertantly crept in.

5b3bd7e6-c3f3-4830-a555-991f5e6beec0 commented 8 years ago

comment:20

Yup. It was a typo. Don't know how it got in there. :P

1861b8a9-77f0-4f35-8431-8514a75b40d1 commented 8 years ago

comment:21

If you're using vim, it might be a :w which ended up as a :3 This kind of stuff happens quite often :)

Anyway, I'm setting this ticket to positive_review.

David

vbraun commented 8 years ago

comment:22

Can you fill out the reviewer field?

1861b8a9-77f0-4f35-8431-8514a75b40d1 commented 8 years ago

comment:23

Can you fill out the reviewer field?

Sorry, done now.

BTW, the patchbot says "tests failed" but it's in a completely unrelated file (intefaces/qepcad.py) so I don't think it's because of this ticket. I'm resetting it to needs_review.

1861b8a9-77f0-4f35-8431-8514a75b40d1 commented 8 years ago

Reviewer: David Lucas

vbraun commented 8 years ago

Changed branch from u/arpitdm/fix_wtdist_gap_method to 191d1c4

sagemath / sage

Fix LinearCode.wtdist_gap method #20565