Opened 10 years ago
Closed 10 years ago
#13723 closed enhancement (fixed)
Moving hamming_weight from sage.coding to sage.modules
Reported by: | tfeulner | Owned by: | wdj |
---|---|---|---|
Priority: | minor | Milestone: | sage-5.6 |
Component: | coding theory | Keywords: | Hamming weight, coding theory |
Cc: | Merged in: | sage-5.6.beta0 | |
Authors: | Thomas Feulner | Reviewers: | Punarbasu Purkayastha, Travis Scrimshaw |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
Hamming weights are defined in sage.coding.linear_codes for vectors. This is the wrong place, since we can not use the implementation specific structure of a vector, for example in the binary case.
This patch adds a function hamming_weight to the class sage.modules.free_module_element.FreeModuleElement? and deprecates sage.coding.linear_codes.hamming_weight.
Apply only: trac_13723-hamming_weight.patch
Attachments (1)
Change History (13)
comment:1 Changed 10 years ago by
comment:2 Changed 10 years ago by
In my opinion, we should deprecate the function hamming_weight() in sage/coding/. Why should we have a global function in an object oriented language as Python is? In my experience as a new developer, this is one of the main problems of Sage that you have several functions doing the same... The same is true for some user who shouldn`t be forced to have a look into the source code to decide which function he/she should use.
Finally, the docstring tells us that v
has to be a vector, so the old code will only work in those cases which are catched by the new patch so I guess that we will only catch an
AttributeError
which we will produce in the following 'old code`.
comment:3 follow-up: 5 Changed 10 years ago by
Ok. I missed the fact that you haven't removed it from global scope.
There are three comments I have about the patch (sorry for nitpicking :)):
- It is missing the hg headers containing your name and other information. Maybe you don't have a hgrc? Also, the patch should have the ticket number in its name (like
trac_13723-hamming_weight.patch
or13273-hamming_weight.patch
). If I am not mistaken Jeroen has some script which parses the patch name to get the ticket number. Since you have already made this patch, you can simply rename it using hg like$ cd <sage_root>/devel/sage $ hg qmv hamming_weight_simplex_code.patch trac_13723-hamming_weight.patch -- OR if you don't have hg installed in your system -- $ sage -hg qmv hamming_weight_simplex_code.patch trac_13723-hamming_weight.patch
- In the example in
vector_mod2_dense.pyx
file, the answer should be 2. - Can you fix the
type(v) == list
statements withisinstance(v, list)
? It is not your mistake, but it would be nice to have them fixed.
Otherwise the patch looks fine to me.
comment:4 Changed 10 years ago by
Authors: | → Thomas Feulner |
---|---|
Description: | modified (diff) |
Status: | new → needs_review |
comment:5 Changed 10 years ago by
Replying to ppurka:
Ok. I missed the fact that you haven't removed it from global scope.
There are three comments I have about the patch (sorry for nitpicking :)):
It is ok, I learned a lot from those comments.
- It is missing the hg headers containing your name and other information. Maybe you don't have a hgrc? Also, the patch should have the ticket number in its name (like
trac_13723-hamming_weight.patch
or13273-hamming_weight.patch
). If I am not mistaken Jeroen has some script which parses the patch name to get the ticket number. Since you have already made this patch, you can simply rename it using hg like$ cd <sage_root>/devel/sage $ hg qmv hamming_weight_simplex_code.patch trac_13723-hamming_weight.patch -- OR if you don't have hg installed in your system -- $ sage -hg qmv hamming_weight_simplex_code.patch trac_13723-hamming_weight.patch
I think it contains all the information now, I worked with hg_sage and for some reason this information was not added.
- In the example in
vector_mod2_dense.pyx
file, the answer should be 2.
Of course, thanks.
- Can you fix the
type(v) == list
statements withisinstance(v, list)
? It is not your mistake, but it would be nice to have them fixed.
Done.
Otherwise the patch looks fine to me.
Thanks again, are there other comments?
comment:7 Changed 10 years ago by
Status: | needs_review → positive_review |
---|
Looks good to me, though I am not particularly fond of using "self" in documentation. Anyway, positive review from my side. Everything seems to work here.
comment:8 Changed 10 years ago by
Milestone: | sage-5.6 → sage-5.5 |
---|
comment:9 follow-up: 10 Changed 10 years ago by
Reviewers: | → Punarbasu Purkayastha |
---|---|
Status: | positive_review → needs_work |
Sorry but it's my turn to be nitpicking. There's some documentation things I would like to see addressed:
- It should be
:trac:`13723`
, the#
is added automatically and as you have it written, the link is incorrect as it includes the#
. - I prefer to see everything set it code format/linked as much as possible:
- In the authors section:
Added :meth:`CLASSNAME.hamming_weight`
orAdded ``hamming_weight()``
- Change all appropriate lines:
Return the number of positions ``i`` such that ``self[i] != 0``.
- In the authors section:
- I don't like the deprecation message. Perhaps something like
The global function hamming_weight(v) is deprecated, instead use v.hamming_weight().
Thanks,
Travis
Changed 10 years ago by
Attachment: | trac_13723-hamming_weight.patch added |
---|
comment:10 Changed 10 years ago by
Status: | needs_work → needs_review |
---|
comment:11 Changed 10 years ago by
Milestone: | sage-5.5 → sage-5.6 |
---|---|
Reviewers: | Punarbasu Purkayastha → Punarbasu Purkayastha, Travis Scrimshaw |
Status: | needs_review → positive_review |
Looks good to me.
comment:12 Changed 10 years ago by
Merged in: | → sage-5.6.beta0 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
You need to deprecate the top level function (which you have already done) and not remove it entirely. The removal can happen after the deprecation period is over (typically 1 year).
Anyway, the question is whether it should be deprecated. There is nothing wrong with having it both as
v.hamming_weight()
and as a global functionhamming_weight(v)
. In the latter case, you could modify the function to use the attribute if it is available: