Opened 10 years ago

Closed 8 years ago

#12418 closed enhancement (fixed)

adding Delsarte bound for codes

Reported by: dimpase Owned by: wdj
Priority: major Milestone: sage-5.12
Component: coding theory Keywords:
Cc: jpang, kini, wdj, ncohen, ppurka, ptrrsn_1 Merged in: sage-5.12.beta1
Authors: Dmitrii Pasechnik Reviewers: Frédéric Chapoton, Punarbasu Purkayastha
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #12533, #13650 Stopgaps:

Status badges

Description (last modified by dimpase)

Delsarte bound for codes, aka Linear Programming bound, is easy to implement in Sage.

To work well in big dimensions, one needs an arbitrary precision LP solver. We use an LP backend to PPL, which is available in Sage since #12533.

Apply

Attachments (6)

delsarte_bound.py (6.7 KB) - added by dimpase 9 years ago.
a prototype implementation
update.diff (2.8 KB) - added by dimpase 9 years ago.
update of the patch - for reviewing only
12418_delsart_bounds.2.patch (20.5 KB) - added by dimpase 8 years ago.
rebased for Sage 5.10 and fixed some outstanding issues
12418_delsart_bounds.patch (20.5 KB) - added by dimpase 8 years ago.
rebased for Sage 5.10 and fixed some outstanding issues
12418_refactored.diff (3.0 KB) - added by dimpase 8 years ago.
code refactoring - for review only
12418_update.patch (3.8 KB) - added by dimpase 8 years ago.
update to fix the remaining issues

Download all attachments as: .zip

Change History (47)

comment:1 Changed 10 years ago by dimpase

Last edited 9 years ago by dimpase (previous) (diff)

comment:2 Changed 10 years ago by dimpase

  • Cc kini added

comment:3 Changed 10 years ago by dimpase

  • Cc wdj added

comment:4 Changed 10 years ago by dimpase

  • Cc ncohen added
  • Description modified (diff)

comment:5 Changed 10 years ago by ncohen

(GLPK can solve non-integer rational LP. It is not exposed, but may not be too hard either)

comment:6 Changed 10 years ago by dimpase

  • Cc ppurka added

comment:7 Changed 10 years ago by ppurka

The function named delsarte_bound should be renamed to something like delsarte_bound_hamming_space. This is so that in future other functions like delsarte_bound_johnson_space, delsarte_bound_permutation_space, etc can be added easily, without having inconsistencies in naming.

comment:8 Changed 10 years ago by dimpase

  • Cc ptrrsn_1 added

comment:9 Changed 9 years ago by dimpase

  • Description modified (diff)

comment:10 Changed 9 years ago by dimpase

  • Dependencies set to #12533
  • Description modified (diff)

Changed 9 years ago by dimpase

a prototype implementation

comment:11 Changed 9 years ago by dimpase

  • Description modified (diff)
  • Status changed from new to needs_review

comment:12 follow-ups: Changed 9 years ago by ppurka

I think the Krawtchouk polynomial could be computed explicitly by not making repeated calls to binomial. This should speed it up. I have something like this in mind:

def Krawtchouk2(n,q,l,i):
    # Use the expression in equation (55) of MacWilliams & Sloane, pg 151
    # We write jth term = some_factor * (j-1)th term
    kraw = jth_term = (q-1)**l * binomial(n, l) # j=0
    for j in range(1,l+1):
        jth_term *= -q*(l-j+1)*(i-j+1)/((q-1)*j*(n-j+1))
        kraw += jth_term
    return kraw

n,q,l,i = 10,8,7,5
timeit('Krawtchouk2(n,q,l,i)')
timeit('Krawtchouk (n,q,l,i)')
print Krawtchouk2(n,q,l,i) == Krawtchouk(n,q,l,i)

625 loops, best of 3: 53.3 µs per loop
625 loops, best of 3: 205 µs per loop
True

I noticed that sage handles nonintegral components in the binomial, so the expression for the Krawtchouk already works with nonintegral n and x.

n,q,l,i = 10.6,8,7,5.4
#timeit('Krawtchouk3(n,q,l,i)')
timeit('Krawtchouk2(n,q,l,i)')
timeit('Krawtchouk (n,q,l,i)')
print Krawtchouk2(n,q,l,i) == Krawtchouk(n,q,l,i)
print Krawtchouk2(n,q,l,i), Krawtchouk(n,q,l,i)

625 loops, best of 3: 382 µs per loop
125 loops, best of 3: 4.74 ms per loop
False
93582.0160001147 93582.0159999999

comment:13 Changed 9 years ago by ppurka

Can you mention when it guarantees a weight spectrum? Would doing an ILP make it a proper weight spectrum?

   - ``return_data`` -- if ``True``, return a weights vector, which actually need not 
     be a proper weight enumerator, or even have integer entries, and the LP. 

Also, I think the term weight enumerator refers to the weight enumerator polynomial. Perhaps using weight distribution or distance distribution is more appropriate here.

comment:14 in reply to: ↑ 12 ; follow-up: Changed 9 years ago by dimpase

Replying to ppurka:

I think the Krawtchouk polynomial could be computed explicitly by not making repeated calls to binomial. This should speed it up.

It's probably even faster to compute by using recurrence relations, but I don't think it's important here: LP solving timing clearly dominates the rest.

By the way, would it be interesting to include an option to compute bounds on codes with a prescribed forbidden set of distances, rather than just [1..d] ? It's a trivial add-on. I did this in a prototype code for Johnson schemes, here.

Any other interesting schemes to include? (Johnson scheme takes care of constant weight binary codes, as you know.)

comment:15 Changed 9 years ago by dimpase

  • Dependencies changed from #12533 to #12533, #13650
  • Description modified (diff)
  • Milestone changed from sage-5.4 to sage-5.5

comment:16 in reply to: ↑ 14 ; follow-up: Changed 9 years ago by ppurka

Replying to dimpase:

Replying to ppurka:

I think the Krawtchouk polynomial could be computed explicitly by not making repeated calls to binomial. This should speed it up.

It's probably even faster to compute by using recurrence relations, but I don't think it's important here: LP solving timing clearly dominates the rest.

The point is that someone might try to use these polynomials more generally in a separate context. They are not defined anywhere else in Sage, so anyone who tries to use them will use this one.

By the way, would it be interesting to include an option to compute bounds on codes with a prescribed forbidden set of distances, rather than just [1..d] ? It's a trivial add-on. I did this in a prototype code for Johnson schemes, here.

Wow! You have the Johnson scheme too?! Sure, add them all in!! Do you use the polynomials used by Aaltonen?

Any other interesting schemes to include? (Johnson scheme takes care of constant weight binary codes, as you know.)

LP for permutation codes would be interesting. There are not too many good results known there. IIRC, it uses Charlier polynomials.

EDIT: FWIW, it is Charlier polynomials.

Last edited 9 years ago by ppurka (previous) (diff)

comment:17 Changed 9 years ago by ppurka

Oh, I forgot to add. Forbidden distances will be nice as well. I think only some special cases achieve the closed form solutions. In general, still not much is known. It looks like you only need to drop distances d_1,...,d_m instead of 1,...,d, right?

How about introducing an extra keyword called forbidden_distances or exclude_distances, which defaults to 1,...,d?

comment:18 in reply to: ↑ 16 ; follow-up: Changed 9 years ago by dimpase

Replying to ppurka:

Replying to dimpase:

Replying to ppurka:

I think the Krawtchouk polynomial could be computed explicitly by not making repeated calls to binomial. This should speed it up.

It's probably even faster to compute by using recurrence relations, but I don't think it's important here: LP solving timing clearly dominates the rest.

The point is that someone might try to use these polynomials more generally in a separate context. They are not defined anywhere else in Sage, so anyone who tries to use them will use this one.

Actually, I have most discrete orthogonal polynomials arising in the classical P- and Q- polynomial schemes implemented, although it's neither polished nor optimized.

[

By the way, would it be interesting to include an option to compute bounds on codes with a prescribed forbidden set of distances, rather than just [1..d] ? It's a trivial add-on. I did this in a prototype code for Johnson schemes, here.

Wow! You have the Johnson scheme too?! Sure, add them all in!! Do you use the polynomials used by Aaltonen?

I use the descriptions in the book "Algebraic Combinatorics I" by E.Bannai and T.Ito. Something known as Eberlein polynomials.

Any other interesting schemes to include? (Johnson scheme takes care of constant weight binary codes, as you know.)

LP for permutation codes would be interesting. There are not too many good results known there. IIRC, it uses Chebychev polynomials(?).

yes, this should be perfectly doable.

comment:19 in reply to: ↑ 18 Changed 9 years ago by ppurka

Replying to dimpase:

I use the descriptions in the book "Algebraic Combinatorics I" by E.Bannai and T.Ito. Something known as Eberlein polynomials.

That's for the binary case. For the q-ary case it is a product of Krawtchouk and Hahn, if I recall properly. Let me fish out the paper; I will send it to you.

comment:20 in reply to: ↑ 12 ; follow-up: Changed 9 years ago by dimpase

Replying to ppurka:

I think the Krawtchouk polynomial could be computed explicitly by not making repeated calls to binomial. This should speed it up. I have something like this in mind:

This can be further optimized by using Horner's rule. I'll do this, and leave the rest (other schemes) for another ticket, OK?

comment:21 in reply to: ↑ 20 Changed 9 years ago by ppurka

Replying to dimpase:

This can be further optimized by using Horner's rule. I'll do this, and leave the rest (other schemes) for another ticket, OK?

Yes, yes. One space/polynomial at a time. Just Hamming space in this ticket is OK.

Changed 9 years ago by dimpase

update of the patch - for reviewing only

comment:22 Changed 9 years ago by dimpase

  • Description modified (diff)

Please review. I added a Kravchouck speedup, and cleaned up docstrings as requested.

comment:23 Changed 9 years ago by dimpase

improved docstrings for return_data

comment:24 follow-ups: Changed 9 years ago by ppurka

Thanks for the update. I have some general comments. Will look into this patch in more detail too.

  1. There are lot of trailing whitespaces. The patchbot will complain. :)
  2. What is the point of this portion of the code? Can't it be replaced by kk = ZZ(log(q, q_base))?
       kk = 0
       while q_base**kk < q:
          kk += 1
    
  3. There is another bit further down:
          m = -1
          while q_base**(m+1) < bd:
            m += 1
          if q_base**(m+1) == bd:
            m += 1
    
  4. Also, I don't think this deprecation is necessary any more. The ticket you cited is over 2 years old.
    -def dimension_upper_bound(n,d,q):
    +@rename_keyword(deprecation=6094, method="algorithm")
    +def dimension_upper_bound(n,d,q,algorithm=None):
    
Version 2, edited 9 years ago by ppurka (previous) (next) (diff)

comment:25 in reply to: ↑ 24 Changed 9 years ago by dimpase

Replying to ppurka:

Thanks for the update. I have some general comments. Will look into this patch in more detail too.

  1. There are lot of trailing whitespaces. The patchbot will complain. :)

I've just uploaded an update with all the trailing spaces removed.

  1. What is the point of this portion of the code? Can't it be replaced by kk = ZZ(log(q, q_base))?
       kk = 0
       while q_base**kk < q:
          kk += 1
    
  2. There is another bit further down:
          m = -1
          while q_base**(m+1) < bd:
            m += 1
          if q_base**(m+1) == bd:
            m += 1
    

this came from an older piece of plain Python. Then I struggled with log() quite a bit, and finally gave up on it and rolled my own.

  1. Also, I don't think this deprecation is necessary any more. The ticket you cited is over 2 years old.
    -def dimension_upper_bound(n,d,q):
    +@rename_keyword(deprecation=6094, method="algorithm")
    +def dimension_upper_bound(n,d,q,algorithm=None):
    

I blindly copied from sage/coding/code_bounds.py Should the whole file be cleaned out of these? By the way, plural methods slipped through this decorator...

Edit: Sorry. It seems ZZ doesn't work but int(log(..)) does work.

comment:26 in reply to: ↑ 24 Changed 9 years ago by dimpase

Replying to ppurka:

Thanks for the update. I have some general comments. Will look into this patch in more detail too.

  1. What is the point of this portion of the code? Can't it be replaced by kk = ZZ(log(q, q_base))?

yes, it is basically what it does; this is also needed to do a Gomory-style cut which might be available due to the corresponding rounding.

comment:27 follow-up: Changed 9 years ago by ppurka

Ok. I am finally getting some time to look into this again. Here are some comments.

  1. Krawtchouk is missing a doctest.
  2. There is no need of \ in def delsarte_bound_...
  3. There are still many trailing whitespaces. If you use vim then you can try this command :%s/[ ]\+$//
  4. - ``q`` -- the length of the alphabet -- this should be "the size of the alphabet"

Also I think the the options q and d can be swapped to be in the order in which they appear in the function definition.

  1. - ``solver`` -- the LP/ILP solved --- this should be solver. What other solvers are present? They should be listed as options in this variable.
  2. The bound on the size of the F_2-codes --- this should be `F_2`
  3. - ``return_data`` --- As it is currently written in the description, it is unclear what this is returning. It looks like the first component is an MIP variable (and not a vector), and the second component is the MILP itself. At a first glance in your doctests, it looks like we need to understand how the MILP works in order to get the values of the weight distribution. Should we just return the weight distribution itself? A weight distribution vector can be returned by doing p.get_values(a).values().
  4. The backend should automatically handle "isinteger=True" at least so that it is functional. Currently I get a very generic Exception (why is this only "Exception"?). Maybe it is automatically handled in a later version of sage? I will have to check against 5.6.beta1 and higher:
    Exception: This backend does not handle integer variables ! Read the doc !
    
  5. Why do we need the second function? We already discussed this off-ticket - I will wait for your generic function.
  6. (**this option is currenly disabled, cf. trac #13667**). --- it should be :trac:`13667` since it will be in the documentation.
  7. Parameter "algorithm" has the same meaning as in codesize_upper_bound() --- This should be :func:`codesize_upper_bound`.
  8. print "Wrong q_base=", q_base, " for q=", q, kk --- This can be formatted python3 style as
          print "Wrong q_base={} for q={} {}".format(q_base, q, kk)
    
  9. According to the patchbot this needs rebasing against some higher version of 5.6. I have only beta0 here; I will check it against rc1 after I have compiled that.
  10. EDIT: I forgot.. the deprecation notice should go. It has been two years. If code_bounds needs to be cleaned then I can do that.

EDIT: Update backticks in 6., 10., 11.

Last edited 9 years ago by ppurka (previous) (diff)

comment:28 Changed 9 years ago by knsam

  • Status changed from needs_review to needs_work

This patch needs to address referee's comments. I am changing this to needs_work in the meanwhile.

Changed 8 years ago by dimpase

rebased for Sage 5.10 and fixed some outstanding issues

comment:29 in reply to: ↑ 27 Changed 8 years ago by dimpase

Replying to ppurka:

rebased to Sage 5.10 and fixed the following:

  1. Krawtchouk is missing a doctest.
  2. There is no need of \ in def delsarte_bound_...
  3. There are still many trailing whitespaces. If you use vim then you can try this command :%s/[ ]\+$//
  4. - ``q`` -- the length of the alphabet -- this should be "the size of the alphabet"

Also I think the the options q and d can be swapped to be in the order in which they appear in the function definition.

I swapped the docstrings instead.

  1. - ``solver`` -- the LP/ILP solved --- this should be solver. What other solvers are present? They should be listed as options in this variable.
  2. The bound on the size of the F_2-codes --- this should be `F_2`

(and other F_)

  1. The backend should automatically handle "isinteger=True" at least so that it is functional. Currently I get a very generic Exception (why is this only "Exception"?). Maybe it is automatically handled in a later version of sage? I will have to check against 5.6.beta1 and higher:

The PPL backend does not handle ILP. One might want to improve the way it reports this error, but not on this ticket.

Exception: This backend does not handle integer variables ! Read the doc !
  1. Parameter "algorithm" has the same meaning as in codesize_upper_bound() --- This should be :func:`codesize_upper_bound`.
  2. According to the patchbot this needs rebasing against some higher version of 5.6. I have only beta0 here; I will check it against rc1 after I have compiled that.

Rebased.

comment:30 follow-up: Changed 8 years ago by dimpase

  • Work issues set to code refactoring

comment:31 in reply to: ↑ 30 Changed 8 years ago by ppurka

Replying to dimpase:

Work issues set to code refactoring

Oh good. I was wondering if you didn't want to do that in this patch.

Changed 8 years ago by dimpase

rebased for Sage 5.10 and fixed some outstanding issues

Changed 8 years ago by dimpase

code refactoring - for review only

comment:32 Changed 8 years ago by dimpase

  • Status changed from needs_work to needs_review
  • Work issues code refactoring deleted

comment:33 Changed 8 years ago by chapoton

instructions for the bot:

apply 12418_delsart_bounds.patch

comment:34 follow-up: Changed 8 years ago by chapoton

  • Status changed from needs_review to needs_work
  • doctest covering is not 100%
  • you can use the wikipedia role for example :wikipedia:`Togo`
  • it would be better to lazy_import the new functions, maybe ?

comment:35 in reply to: ↑ 34 Changed 8 years ago by dimpase

Replying to chapoton:

  • doctest covering is not 100%

hmm, what function do you mean? there is an internal function which is not exported; I don't think it needs a doctest, does it?

  • you can use the wikipedia role for example :wikipedia:`Togo`

where?

  • it would be better to lazy_import the new functions, maybe ?

I have no idea. Is there a stated policy on this? Having said this, perhaps it's better to re-lazy_import the whole sage/coding, something for another ticket?

comment:36 Changed 8 years ago by chapoton

  • well, the bot complains about the missing doctest, so I guess that the internal function needs one indeed
  • instead of `en.wikipedia.org/wiki/Kravchuk_polynomials <http://en.wikipedia.org/wiki/Kravchuk_polynomials>`_

you can write :wikipedia:`Kravchuk_polynomials`

  • the bot is not happy either on adding something new in the global namespace. It is better for the startup time of sage to try and make the bot happy on this point, imho.

Changed 8 years ago by dimpase

update to fix the remaining issues

comment:37 Changed 8 years ago by dimpase

  • Description modified (diff)
  • Status changed from needs_work to needs_review

comment:38 Changed 8 years ago by ppurka

  • Status changed from needs_review to positive_review

The patch looks OK to me now. Though I would have really liked the Q-matrix to be passed to the linear program (for instance to the delsarte LP building function), this can be done in a future patch when LP for other spaces are introduced.

Last edited 8 years ago by ppurka (previous) (diff)

comment:39 Changed 8 years ago by ppurka

  • Authors set to Dmitrii Pasechnik
  • Reviewers set to Frédéric Chapoton, Punarbasu Purkayastha

comment:40 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:41 Changed 8 years ago by jdemeyer

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