Moving hamming_weight from sage.coding to sage.modules

[tfeulner]

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

[ppurka]

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 function hamming_weight(v). In the latter case, you could modify the function to use the attribute if it is available:

def hamming_weight(v): # the function in sage/coding/
    try:
        return v.hamming_weight()
    except AttributeError:
        # the old code comes here

[tfeulner]

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`.

[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 :)):

1. 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 or 13273-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
   

2. In the example in vector_mod2_dense.pyx file, the answer should be 2.
3. Can you fix the type(v) == list statements with isinstance(v, list)? It is not your mistake, but it would be nice to have them fixed.

Otherwise the patch looks fine to me.

[tfeulner]

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.

1. 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 or 13273-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.

2. In the example in vector_mod2_dense.pyx file, the answer should be 2.

Of course, thanks.

3. Can you fix the type(v) == list statements with isinstance(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?

[tfeulner]

Patchbot: apply trac_13723-hamming_weight.patch

[ppurka]

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.

[tscrim]

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` or Added ``hamming_weight()``
  • Change all appropriate lines:

    Return the number of positions ``i`` such that ``self[i] != 0``.
    

• 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

[tfeulner]

Replying to tscrim:

Ok, done. Thanks for your help.

Thomas

[tscrim]

Looks good to me.