#20565 closed defect (fixed)

Fix LinearCode.wtdist_gap method

Reported by: arpitdm Owned by: arpitdm
Priority: minor Milestone: sage-7.3
Component: coding theory Keywords:
Cc: dlucas, jsrn Merged in:
Authors: Arpit Merchant Reviewers: David Lucas
Report Upstream: N/A Work issues:
Branch: 191d1c4 (Commits) Commit: 191d1c4aa70a36e995406abb9d6e5fbb2f96beee
Dependencies: Stopgaps:

Description

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

Attachments (1)

wtdist_gap.txt (1.8 KB) - added by arpitdm 16 months ago.
Notes on the issues with the method and proposed corrections.

Download all attachments as: .zip

Change History (25)

Changed 16 months ago by arpitdm

Notes on the issues with the method and proposed corrections.

comment:1 Changed 16 months ago by arpitdm

  • Owner changed from (none) to arpitdm

comment:2 Changed 15 months ago by arpitdm

  • Branch set to u/arpitdm/fix_wtdist_gap_method

comment:3 Changed 15 months ago by arpitdm

  • Commit set to 3c62c141b1747290a12a3647d6e939bff13a590f
  • Status changed from new to needs_review

New commits:

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

comment:4 Changed 15 months ago by arpitdm

  • Status changed from needs_review to needs_work

comment:5 Changed 15 months ago by git

  • Commit changed from 3c62c141b1747290a12a3647d6e939bff13a590f to 161a2681636d3139a589aafe64f289d982c54bd6

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

161a268placed 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.

comment:6 Changed 15 months ago by arpitdm

  • Status changed from needs_work to needs_review

comment:7 follow-up: Changed 15 months ago by dlucas

  • Milestone changed from sage-7.2 to sage-7.3
  • Status changed from needs_review to needs_work

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

comment:8 in reply to: ↑ 7 ; follow-up: Changed 15 months ago by jsrn

  • 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

comment:9 Changed 15 months ago by arpitdm

  1. Deprecation warnings:

Understood. I'm adding the line now.

  1. 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).

  1. Input to the method:

Agreed. I'll make changes accordingly.

Last edited 15 months ago by arpitdm (previous) (diff)

comment:10 in reply to: ↑ 8 Changed 15 months ago by dlucas

Replying to jsrn:

  • 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

comment:11 Changed 15 months ago by git

  • Commit changed from 161a2681636d3139a589aafe64f289d982c54bd6 to 111426739cd1bb4eb395117ff5000d9aafdc23ac

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

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

comment:12 Changed 15 months ago by dlucas

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

comment:13 Changed 15 months ago by arpitdm

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.

  1. Do we want that spectrum also call wtdist_gap() which then calls _spectrum_from_gap()? Or can spectrum directly call the latter?
  2. 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?

comment:14 Changed 15 months ago by dlucas

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

comment:15 Changed 15 months ago by arpitdm

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]

comment:16 Changed 15 months ago by dlucas

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

comment:17 Changed 15 months ago by git

  • Commit changed from 111426739cd1bb4eb395117ff5000d9aafdc23ac to 10d7ca77d6699ff4877797072b0c5670881b6ab2

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

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

comment:18 Changed 15 months ago by dlucas

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

comment:19 Changed 15 months ago by git

  • Commit changed from 10d7ca77d6699ff4877797072b0c5670881b6ab2 to 191d1c4aa70a36e995406abb9d6e5fbb2f96beee

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

191d1c4fixed small typo that had inadvertantly crept in.

comment:20 Changed 15 months ago by arpitdm

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

comment:21 Changed 15 months ago by dlucas

  • Status changed from needs_work to positive_review

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

comment:22 Changed 15 months ago by vbraun

  • Status changed from positive_review to needs_work

Can you fill out the reviewer field?

comment:23 Changed 15 months ago by dlucas

  • Reviewers set to David Lucas
  • Status changed from needs_work to positive_review

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.

comment:24 Changed 15 months ago by vbraun

  • Branch changed from u/arpitdm/fix_wtdist_gap_method to 191d1c4aa70a36e995406abb9d6e5fbb2f96beee
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.