5
\$\begingroup\$

This program is to return a number one at a time from a given input list of numbers, with a frequency proportional to probability/fraction associated with each number.

I know it could be made a generator function. But if I have to do it inside a class, can you please suggest improvements to this code, particularly with regards to the exception classes I have made below?

from random import random
from bisect import bisect_right

class Invalid_fraction(ValueError): pass
class Negative_fraction(ValueError): pass
class Sum_fraction(ValueError): pass
class NullValues(ValueError): pass

class RandomGen(object):    
    def __init__(self, nums, fractions):
        #Basic error checks
        if ( len(nums)==0 or len(fractions) == 0):
            raise NullValues("Null sample space")

        if ( len(nums) != len(fractions) ):
            raise Invalid_fraction("Invalid fraction list")

        #Data initiallisations
        self._sum  = 0
        self._fractions= []
        self._nums = nums 

    #Allocating a range within 0-1 to each of the input number, along with check for negative fraction.

        for p in fractions:
            if p < 0:
                raise Negative_fraction("negative fraction") 
            self._sum += p
            self._fractions.append(self._sum)

        if self._sum != 1:
            raise Sum_fraction("Sum of fractions is not equal to 1")

    def next_num(self):     
        #Get a random number between 0 and 1 and yield the number of the bucket
        random_number = random()
        index = bisect_right(self._fractions,random_number)
        return self._nums[index]
\$\endgroup\$
4
  • 2
    \$\begingroup\$ Welcome to CodeReview.SE! Can you give us a simple example to know how to use your code ? \$\endgroup\$
    – SylvainD
    Jul 1 '14 at 13:07
  • \$\begingroup\$ r=RandomGen([-2, -1, 0, 1, 2], [0.06, 0.15, 0.53, 0.26]) c=collections.Counter((r.next_num for i in range(100))) print(c) This should give something like this: {0: 55, -1: 25, 1: 19, 2: 1} \$\endgroup\$
    – Papal
    Jul 1 '14 at 13:42
  • \$\begingroup\$ Your example does not work : a value is missing in the second list and parenthesis are required to call the function in your generator expression. \$\endgroup\$
    – SylvainD
    Jul 1 '14 at 13:55
  • \$\begingroup\$ Sorry. r=RandomGen([-2, -1, 0, 1, 2], [0.06, 0.15, 0.53, 0.20,0.06]) >>> c=collections.Counter((r.next_num() for i in range(100))) >>> print(c) Counter({0: 55, 1: 22, -1: 10, -2: 7, 2: 6}) \$\endgroup\$
    – Papal
    Jul 1 '14 at 14:56
3
\$\begingroup\$

Frankly, it seems unnecessary to have any custom exceptions (and if you keep them, they should all have PEP-8-compliant names). All of the custom exceptions could just be ValueErrors ("when a built-in operation or function receives an argument that has the right type but an inappropriate value") with different messages, e.g.:

if not nums or not fractions: # note empty collections evaluate False in a boolean context
    raise ValueError("Null sample space")
\$\endgroup\$
0
2
\$\begingroup\$

Style

Your code does not respect PEP8.

You'll find various tools to check your code and I suggest you use them. After making a few cosmetic changes (spacing, line length, line breaks, etc), your code looks like :

class Invalid_fraction(ValueError):
    pass


class Negative_fraction(ValueError):
    pass


class Sum_fraction(ValueError):
    pass


class NullValues(ValueError):
    pass


class RandomGen(object):
    def __init__(self, nums, fractions):
        #Basic error checks
        if len(nums) == 0 or len(fractions) == 0:
            raise NullValues("Null sample space")

        if len(nums) != len(fractions):
            raise Invalid_fraction("Invalid fraction list")

        # Data initiallisations
        self._sum = 0
        self._fractions = []
        self._nums = nums

        # Allocating a range within 0-1 to each of the input number
        # along with check for negative fraction.
        for p in fractions:
            if p < 0:
                raise Negative_fraction("negative fraction")
            self._sum += p
            self._fractions.append(self._sum)

        if self._sum != 1:
            raise Sum_fraction("Sum of fractions is not equal to 1")

    def next_num(self):
        # Get a random number between 0 and 1 and yield the number of the bucket
        random_number = random()
        index = bisect_right(self._fractions, random_number)
        return self._nums[index]

Class names and and docstrings are still to be fixed bu I'll leave that to you.

Logic

If you check that the 2 inputs have the same length first, you don't have to check that they are both non-empty.

Also, because an empty list of fraction would lead to a sum of 0, this error would be properly caught and signalled to the user so this check might not be useful.

Once your object is properly created, we always have self._sum == 1, thus, there is probably no need to store it in the object.

Your code then becomes :

def __init__(self, nums, fractions):
    #Basic error checks
    if len(nums) != len(fractions):
        raise Invalid_fraction("Invalid fraction list")

    # Data initialisations
    sum_prob = 0
    self._fractions = []
    self._nums = nums

    # Allocating a range within 0-1 to each of the input number
    # along with check for negative fraction.
    for p in fractions:
        if p < 0:
            raise Negative_fraction("negative fraction")
        sum_prob += p
        self._fractions.append(sum_prob)

    if sum_prob != 1:
        raise Sum_fraction("Sum of fractions is not equal to 1")

Responsability

Your RandomGen becomes some kind of way to go through nums. The problem is that the list might get updated after the generator got created. You could consider that this is an non-issue because this is not a valid way to use your class or you could try to make things differently. I can see two options for this :

  • copy the list during the creation
  • consider that the generator does not need to be aware of the list at all : we just generate indices and then one can use it on any container he fancies.

I prefer this second option as it would make your code simpler and also give the class a single responsibility : generating random indices.

class RandomGen(object):
    def __init__(self, fractions):
        # Data initialisations
        sum_prob = 0
        self._fractions = []

        # Allocating a range within 0-1 to each of the input number
        # along with check for negative fraction.
        for p in fractions:
            if p < 0:
                raise Negative_fraction("negative fraction")
            sum_prob += p
            self._fractions.append(sum_prob)

        if sum_prob != 1:
            raise Sum_fraction("Sum of fractions is not equal to 1")

    def next_num(self):
        # Get a random number between 0 and 1 and yield the number of the bucket
        random_number = random()
        return bisect_right(self._fractions, random_number)

Going further

Working with floating numbers and equal sum can be a bit awkward. Indeed, if I wanted to generate indices over n equal parts, I'd use an input like : [1./n]*n. This works fine when n is 2 or 5 but it fails when n is 3. An easy way to make your class easier to use is to say instead of checking if a condition is met, I'll personnally make sure the condition becomes true.. In your case, you could divise all numbers by the sum so that the new sum becomes (roughtly) 1.

def __init__(self, fractions):
    sum_prob = sum(fractions)
    if sum_prob == 0:
        raise Sum_fraction("Sum is zero")

    # Allocating a range within 0-1 to each of the input number
    # along with check for negative fraction.
    self._fractions = []
    partial_sum_prob =  0
    for p in fractions:
        if p < 0:
            raise Negative_fraction("negative fraction")
        p /= sum_prob
        partial_sum_prob += p
        self._fractions.append(partial_sum_prob)
\$\endgroup\$
1
  • \$\begingroup\$ Many Thanks Josay for your comments. They are really useful. :) \$\endgroup\$
    – Papal
    Jul 2 '14 at 3:48

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.