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:

Status badges

Description (last modified by 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

Attachments (1)

trac_13723-hamming_weight.patch (10.3 KB) - added by tfeulner 9 years ago.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 9 years ago by 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

comment:2 Changed 9 years ago by 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`.

comment:3 follow-up: Changed 9 years ago by 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.

comment:4 Changed 9 years ago by tfeulner

  • Authors set to Thomas Feulner
  • Description modified (diff)
  • Status changed from new to needs_review

comment:5 in reply to: ↑ 3 Changed 9 years ago by 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.

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

Of course, thanks.

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

comment:6 Changed 9 years ago by tfeulner

Patchbot: apply trac_13723-hamming_weight.patch

comment:7 Changed 9 years ago by ppurka

  • 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 tfeulner

  • Milestone changed from sage-5.6 to sage-5.5

comment:9 follow-up: Changed 9 years ago by tscrim

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

Changed 9 years ago by tfeulner

comment:10 in reply to: ↑ 9 Changed 9 years ago by tfeulner

  • Status changed from needs_work to needs_review

Replying to tscrim:

Ok, done. Thanks for your help.

Thomas

comment:11 Changed 9 years ago by tscrim

  • 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 jdemeyer

  • Merged in set to sage-5.6.beta0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.