Opened 7 years ago
Closed 7 years ago
#20565 closed defect (fixed)
Fix LinearCode.wtdist_gap method
Reported by:  Arpit Merchant  Owned by:  Arpit Merchant 

Priority:  minor  Milestone:  sage7.3 
Component:  coding theory  Keywords:  
Cc:  David Lucas, Johan Rosenkilde  Merged in:  
Authors:  Arpit Merchant  Reviewers:  David Lucas 
Report Upstream:  N/A  Work issues:  
Branch:  191d1c4 (Commits, GitHub, GitLab)  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)
Change History (25)
Changed 7 years ago by
Attachment:  wtdist_gap.txt added 

comment:1 Changed 7 years ago by
Owner:  set to Arpit Merchant 

comment:2 Changed 7 years ago by
Branch:  → u/arpitdm/fix_wtdist_gap_method 

comment:3 Changed 7 years ago by
Commit:  → 3c62c141b1747290a12a3647d6e939bff13a590f 

Status:  new → needs_review 
New commits:
3c62c14  improved name and documentation of the wtdist_gap method. made this a private method.

comment:4 Changed 7 years ago by
Status:  needs_review → needs_work 

comment:5 Changed 7 years ago by
Commit:  3c62c141b1747290a12a3647d6e939bff13a590f → 161a2681636d3139a589aafe64f289d982c54bd6 

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.

comment:6 Changed 7 years ago by
Status:  needs_work → needs_review 

comment:7 followup: 8 Changed 7 years ago by
Milestone:  sage7.2 → sage7.3 

Status:  needs_review → 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 passn
andF
(asself.length()
andself.base_ring()
will recovern
andF
) 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 followup: 10 Changed 7 years ago by
 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 helperfunction 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 7 years ago by
 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.
comment:10 Changed 7 years ago by
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 helperfunction for
AbstractLinearCode.spectrum
which has the aliasAbstractLinearCode.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 inspectrum
. That would make sense.Best, Johan
Yes, you're right, please ignore my remarks on this!
David
comment:11 Changed 7 years ago by
Commit:  161a2681636d3139a589aafe64f289d982c54bd6 → 111426739cd1bb4eb395117ff5000d9aafdc23ac 

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

comment:12 Changed 7 years ago by
Hello,
There's still a few problems:
 You deleted some lines from
spectrum
, especially the ones defining the local variables (likeF = self.base_ring()
)... Which results in several failures in this method (l. 3306, for instance, returnsglobal 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)
inAbstractLinearCode
, it does not respect the old signature anymore. With the deprecation mechanism working as expected, one should be able to callsage: 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 callsC.wtdist_gap(Gstr, 7, F)
becauseC._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 7 years ago by
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 canspectrum
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?
comment:14 Changed 7 years ago by
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 callwtdist_gap()
which then calls_spectrum_from_gap()
? Or canspectrum
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 7 years ago by
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/sage7.1/src/bin/sageipython: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/sage7.1/local/lib/python2.7/sitepackages/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/sage7.1/local/lib/python2.7/sitepackages/IPython/core/formatters.py:98: DeprecationWarning: DisplayFormatter._formatters_default is deprecated: use @default decorator instead. def _formatters_default(self): /home/arpit/Documents/GSOC_16/sage7.1/local/lib/python2.7/sitepackages/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/sage7.1/local/lib/python2.7/sitepackages/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/sage7.1/local/lib/python2.7/sitepackages/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/sage7.1/local/lib/python2.7/sitepackages/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/sage7.1/local/lib/python2.7/sitepackages/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/sage7.1/local/lib/python2.7/sitepackages/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 7 years ago by
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 7 years ago by
Commit:  111426739cd1bb4eb395117ff5000d9aafdc23ac → 10d7ca77d6699ff4877797072b0c5670881b6ab2 

Branch pushed to git repo; I updated commit sha1. New commits:
10d7ca7  reinstated the input, signatures and output of the nowdeprecated wtdist_gap method.

comment:18 Changed 7 years ago by
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 7 years ago by
Commit:  10d7ca77d6699ff4877797072b0c5670881b6ab2 → 191d1c4aa70a36e995406abb9d6e5fbb2f96beee 

Branch pushed to git repo; I updated commit sha1. New commits:
191d1c4  fixed small typo that had inadvertantly crept in.

comment:21 Changed 7 years ago by
Status:  needs_work → 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 7 years ago by
Status:  positive_review → needs_work 

Can you fill out the reviewer field?
comment:23 Changed 7 years ago by
Reviewers:  → David Lucas 

Status:  needs_work → 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 7 years ago by
Branch:  u/arpitdm/fix_wtdist_gap_method → 191d1c4aa70a36e995406abb9d6e5fbb2f96beee 

Resolution:  → fixed 
Status:  positive_review → closed 
Notes on the issues with the method and proposed corrections.