Opened 11 years ago

Closed 10 years ago

#12013 closed defect (fixed)

Fix entropy function in devel/sage/sage/coding

Reported by: ppurka Owned by: wdj
Priority: minor Milestone: sage-5.0
Component: coding theory Keywords: entropy coding theory
Cc: wdj, kini, jpang Merged in: sage-5.0.beta7
Authors: Punarbasu Purkayastha Reviewers: Daniel Krenn
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description

The entropy function doesn't take care of corner cases when it is evaluated at 0 and 1, and neither does it check whether the range is within 0 and 1.

Attached patched passes all tests in devel/sage/sage/coding

Attachments (1)

trac_12013-fix_entropy.patch (1.8 KB) - added by ppurka 10 years ago.
apply to devel/sage

Download all attachments as: .zip

Change History (12)

comment:1 Changed 11 years ago by ppurka

  • Priority changed from major to minor

comment:2 Changed 10 years ago by ppurka

  • Status changed from new to needs_review

comment:3 Changed 10 years ago by dimpase

  • Cc wdj added

comment:4 Changed 10 years ago by dimpase

  • Cc kini jpang added

comment:5 Changed 10 years ago by dkrenn

  • Status changed from needs_review to needs_work

There should be a doctest for the inputs x=0 and x=1.

Maybe the message of

raise ValueError("The entropy function is undefined for values not between 0 and 1") 

should be more precise (since there is more than one input parameter), e.g. something like

"The value x (x = %s) must be between 0 and 1." % x

Can you also improve the documentation by describing the input parameters x and q and add one or two examples?

I'm not sure, whether it should also be checked that q is positive.

comment:6 Changed 10 years ago by dkrenn

  • Reviewers set to Daniel Krenn

comment:7 Changed 10 years ago by ppurka

  • Status changed from needs_work to needs_review

Thanks for your comments. Updated the patch.

comment:8 Changed 10 years ago by dkrenn

  • Status changed from needs_review to needs_work

Looks much better now. It would be nice to follow the conventions (somewhere in the middle there is an example def point... (...template when documenting functions)). E.g. use ` and `` at the places where variables or formulas (interval) are.

Further I suggest to put sage: entropy(1.1, 2) and sage: entropy(1, 1) under a separate section TESTS::, since to me it seems more like a test than an example showing some functionality.

Maybe you can also add the following example (change the values if you want others) to EXAMPLES:

sage: entropy(1/5,4)
-1/5*log(1/5)/log(4) - 4/5*log(4/5)/log(4) + 1/5*log(3)/log(4)

so that we have an example (with values that are not a special case).

Changed 10 years ago by ppurka

apply to devel/sage

comment:9 Changed 10 years ago by ppurka

  • Status changed from needs_work to needs_review

Updated it again! Didn't know that I was so inconsistent. :)

comment:10 Changed 10 years ago by dkrenn

  • Authors set to Punarbasu Purkayastha
  • Status changed from needs_review to positive_review

comment:11 Changed 10 years ago by jdemeyer

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