Opened 9 years ago
Closed 9 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 9 years ago by
comment:2 Changed 9 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 9 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 9 years ago by
- Description modified (diff)
- Status changed from new to needs_review
comment:5 in reply to: ↑ 3 Changed 9 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:6 Changed 9 years ago by
Patchbot: apply trac_13723-hamming_weight.patch
comment:7 Changed 9 years ago by
- Status changed from needs_review to 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 9 years ago by
- Milestone changed from sage-5.6 to sage-5.5
comment:9 follow-up: ↓ 10 Changed 9 years ago by
- Reviewers set to Punarbasu Purkayastha
- Status changed from positive_review to 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 9 years ago by
comment:10 in reply to: ↑ 9 Changed 9 years ago by
- Status changed from needs_work to needs_review
comment:11 Changed 9 years ago by
- Milestone changed from sage-5.5 to sage-5.6
- Reviewers changed from Punarbasu Purkayastha to Punarbasu Purkayastha, Travis Scrimshaw
- Status changed from needs_review to positive_review
Looks good to me.
comment:12 Changed 9 years ago by
- Merged in set to sage-5.6.beta0
- Resolution set to fixed
- Status changed from positive_review to 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: