Simple arithmetic in Python

Question

As a beginner, I wrote the following python script that solves warrant 1 of this document - pp 436-438.

My solution, although works, seems to me as poorly designed and highly unmaintanable. I was thinking of putting talbe 4C-1 condition A/B as numpy arrays and use mapping to interpolate. I would also like to get rid of my "standard" ifs in the document.

How would you recommend I refactor the code so it is "professionally", more eficient, shorter, and of all correctly done.

Thanks

"""
Traffic Warrant 1
"""



def is_warranted_1(pop, spd_maj, vph_maj, vph_min,
    lanes_maj, lanes_min, standard):

    if standard == 1:
        #Standard 1: (condition A) or (Condition B)
        #8 hours used for condition a could be different than condition B
        if lanes_maj == lanes_min == 1:
            return (vph_maj >= 500 and vph_min >= 150) or (vph_maj >= 750 and vph_min >= 75)
        if lanes_maj >= 2 and lanes_min == 1:
            return (vph_maj >= 600 and vph_min >= 150) or (vph_maj >= 900 and vph_min >= 75)
        if lanes_maj >= lanes_min >= 2:
            return (vph_maj >= 600 and vph_min >= 200) or (vph_maj >= 900 and vph_min >= 100)
        if lanes_maj == 1 and lanes_min >= 2:
            return (vph_maj >= 500 and vph_min >= 200) or (vph_maj >= 750 and vph_min >= 100)

    if standard == 2:
        #Standard 2: (condition A) and (Condition B)
        if lanes_maj == lanes_min == 1:
            return (vph_maj >= 400 and vph_min >= 120) and (vph_maj >= 600 and vph_min >= 60)
        if lanes_maj >= 2 and lanes_min == 1:
            return (vph_maj >= 480 and vph_min >= 120) and (vph_maj >= 720 and vph_min >= 60)
        if lanes_maj >= lanes_min >= 2:
            return (vph_maj >= 480 and vph_min >= 160) and (vph_maj >= 720 and vph_min >= 80)
        if lanes_maj == 1 and lanes_min >= 2:
            return (vph_maj >= 500 and vph_min >= 160) and (vph_maj >= 600 and vph_min >= 80)

    if standard == 3:
        #Standard 3: rural area / Condition A or condition B
        if pop <= 10000 or spd_maj >= 40:
            if lanes_maj == lanes_min == 1:
                return (vph_maj >= 350 and vph_min >= 105) or (vph_maj >= 525 and vph_min >= 53)
            if lanes_maj >= 2 and lanes_min == 1:
                return (vph_maj >= 420 and vph_min >= 105) or (vph_maj >= 630 and vph_min >= 53)
            if lanes_maj >= lanes_min >= 2:
                return (vph_maj >= 420 and vph_min >= 140) or (vph_maj >= 630 and vph_min >= 70)
            if lanes_maj == 1 and lanes_min >= 2:
                return (vph_maj >= 350 and vph_min >= 140) or (vph_maj >= 525 and vph_min >= 70)

    if standard == 4:
        #Standard 4: rural area / last measure / Condition A and Condition B
        if pop <= 10000 or spd_maj >= 40:
            if lanes_maj == lanes_min == 1:
                return (vph_maj >= 280 and vph_min >= 84) and (vph_maj >= 420 and vph_min >= 42)
            if lanes_maj >= 2 and lanes_min == 1:
                return (vph_maj >= 336 and vph_min >= 84) and (vph_maj >= 504 and vph_min >= 42)
            if lanes_maj >= lanes_min >= 2:
                return (vph_maj >= 336 and vph_min >= 112) and (vph_maj >= 504 and vph_min >= 56)
            if lanes_maj == 1 and lanes_min >= 2:
                return (vph_maj >= 280 and vph_min >= 112) and (vph_maj >= 420 and vph_min >= 56)




import random
def test():
    for i in range(1, 20):

        pop = random.choice(range(5000,100000, 500))
        spd_maj = random.choice(range(35,65, 5))
        vph_maj = random.choice(range(200,2000, 50))
        vph_min = random.choice(range(20,1000, 25))
        lanes_maj = random.choice(range(1,4))
        lanes_min = random.choice(range(1,4))
        standard = random.choice(range(1,4))
        result = is_warranted_1(pop, spd_maj, vph_maj, vph_min,
            lanes_maj, lanes_min, standard)
        print "pop: %s, spd_mmaj: %s, vph_maj: %s, vph_min: %s, lanes_maj: %s, \
lanes_min: %s, stnadard: %s. the result is %s" % (pop, spd_maj, 
                vph_maj, vph_min, lanes_maj, lanes_min, standard, result)


if __name__ == "__main__":
    test()

Answer 1

This is the first long answer I've ever written, so bear with me if the writing isn't the best. I've tried to convey my thought process and analysis of the problem.

First, notice that each of the two values you're comparing against (Vehicles per hour on major street, Vehicles per hour on higher-volume minor-street approach) depend ONLY on the respective number of lanes. For example, in standard one, condition A, if lanes_maj == 1, then we only care about vph_maj >= 500. You don't need four separate conditions for this; just figure out what your limits are and then do one comparison:

vph_maj_limit_a = 500 if lanes_maj == 1 else 600
vph_min_limit_a = 150 if lanes_min == 1 else 200
vph_maj_limit_b = 750 if lanes_maj == 1 else 900
vph_min_limit_b = 75 if lanes_min == 1 else 100
... etc ...
return (vph_maj >= vph_maj_limit_a and vph_min >= vph_min_limit_a) and ... etc ...

Next, since all the logic that checks if the vph's are above their limits is the same for all standards, we could use an array instead of a literal value, and index on the standard. This requires that we turn the standard into a 0-based index, which isn't hard, we just subtract 1. Also, in the example I gave, each entry in the list is a 2-tuple; the first one for if the number of lanes is 1, the second if the number of lanes is greater than 1. So we need to calculate the index, which is also easy, and we only need to do it once at the top of the function:

condition_a_maj_limits = ((500, 600), (400, 480), (350, 420), (280, 336))

def blah(...whatever...):

    maj_index = 0 if lanes_maj == 1 else 1
    standard_index = standard - 1
    ...
    vph_maj_limit_a = condition_a_maj_limits[standard_index][maj_index]
    ... etc ...
    return (vph_maj >= vph_maj_limit_a and vph_min >= vph_min_limit_a) and ... etc ...

Now, I only wrote out the limits for vph on the major street for condition A, and that was a pain. But notice that the percentages for each limit in the chart are literally percentages of the max limit. I.e., the 80% limit is 80% of the 100% limit. So we could programmatically create the chart. This might be problematic if the chart changes to add an exception or something, but for now, it's regular.

percentages = (1, 0.8, 0.7, 0.56)
condition_a_maj_limits = [(500*percentage, 600*percentage) for percentage in percentages]

I've used a list comprehension to quickly create the list based on the percentages and the 100% value. Notice that the only difference between each limits list is the condition and whether or not it's major/minor. And all that's really used to generate that list is the pair of values for 100%; first the one where lanes_maj == 1 and then the one for more than 1 lane. So we should write a function to reuse the code instead of copy/pasting it:

def gen_limit_list(one_lane_limit, multi_lane_limit):
    return [(one_lane_limit*p, multi_lane_limit*p) for p in percentages]

...
condition_a_maj_limits = gen_limit_list(500, 600)

We might as well make a function for each of the conditions, which only depend on the standard, the number of lanes, and the vph's:

def condition_a(standard, vph_maj, vph_min, lanes_maj, lanes_min):
    maj_index = 0 if lanes_maj == 1 else 1
    min_index = 0 if lanes_min == 1 else 1
    standard_index = standard - 1
    maj_limit = condition_a_maj_limits[standard_index][maj_index]
    min_limit = condition_a_min_limits[standard_index][min_index]
    return vph_maj >= maj_limit and vph_min >= min_limit

Each different standard has a unique combination of whether or not we check pop, etc, and whether or not we need both conditions or at least one (and vs or). So let's make a separate function for each.

def standard_1(pop, spd_maj, vph_maj, vph_min, lanes_maj, lanes_min):
    cond_a = condition_a(1, vph_maj, vph_min, lanes_maj, lanes_min)
    cond_b = condition_b(1, vph_maj, vph_min, lanes_maj, lanes_min)
    return cond_a or cond_b

def standard_4(pop, spd_maj, vph_maj, vph_min, lanes_maj, lanes_min):
    if pop <= 10000 or spd_maj >= 40:
        cond_a = condition_a(1, vph_maj, vph_min, lanes_maj, lanes_min)
        cond_b = condition_b(1, vph_maj, vph_min, lanes_maj, lanes_min)
        return cond_a and cond_b
    else:
        return False

Might as well get those conditions from a function to avoid having those long ugly lines in four places:

def get_conditions(standard, vph_maj, vph_min, lanes_maj, lanes_min):
    cond_a = condition_a(standard, vph_maj, vph_min, lanes_maj, lanes_min)
    cond_b = condition_b(standard, vph_maj, vph_min, lanes_maj, lanes_min)
    return cond_a, cond_b

def standard_1(pop, spd_maj, vph_maj, vph_min, lanes_maj, lanes_min):
    cond_a, cond_b = get_conditions(1, vph_maj, vph_min, lanes_maj, lanes_min)
    return cond_a or cond_b

Since all our standard functions have the same parameter list, we can put them in a list and dynamically select the right one:

standards_list = (standard_1, standard_2,...etc)

Then our whole warrant function ends up looking like this:

def is_warranted_1(pop, spd_maj, vph_maj, vph_min,
    lanes_maj, lanes_min, standard):

    return standards_list[standard-1](pop, spd_maj, vph_maj, vph_min, lanes_maj, lanes_min)

Here's the final code after some more reductions. This approach is better because it captures the process of looking up the values in the table and comparing them to your parameters rather than just writing out every possibility. It's also much easier to maintain, because if one of the values in the table changes, you only need to change it in one place. Also, very little code is repeated; code duplication is evil.

from collections import namedtuple

# Each standard uses a different set of pairs of vehicle per hour limits, 
# each of which are a certain percentage of a base limit pair.

percentages = (1.00, .80, .70, .56)

def gen_limit_list(single_lane_limit, multi_lane_limit):
        return tuple((single_lane_limit*p, multi_lane_limit*p) for p in percentages)

# Each condition has a different pair of limits for major and minor streets depending on the standard.
limits_a_major = gen_limit_list(500, 600)
limits_a_minor = gen_limit_list(150, 200)
limits_b_major = gen_limit_list(750, 900)
limits_b_minor = gen_limit_list(75, 100)

Params = namedtuple("Params", ("standard", "vph_major", "vph_minor", "lanes_major", "lanes_minor", "pop", "spd_major"))

def get_limit(num_lanes, limits, standard):
    # Arrays are 0-indexed, so Standard 1 is at index 0 and so on.
    standard_index = p.standard - 1
    # Limits is a list of pairs of limits, such as what gen_limit_list returns.
    index = 0 if num_lanes == 1 else 1
    limit = limits[standard_index][index]
    return limit

def get_condition(p, limits_major, limits_minor):
    major_limit = get_limit(p.lanes_major, limits_major, p.standard)
    minor_limit = get_limit(p.lanes_minor, limits_minor, p.standard)
    return p.vph_major >= major_limit and p.vph_minor >= minor_limit

def is_warranted_1(p):
    cond_a = get_condition(p, limits_a_major, limits_a_minor)
    cond_b = get_condition(p, limits_b_major, limits_b_minor)
    # If standard is 1 or 2, then we ignore whether or not it's rural.
    # Standards 1 and 3 require either condition to be true; standards 2 and 4 require both conditions to be true.
    is_rural = p.pop <= 10000 or p.spd_major >= 40
    if p.standard in (1, 2) or is_rural: 
        if p.standard in (1, 3):
            return cond_a or cond_b
        else:
            return cond_a and cond_b
    else:
        # This is reached only when the city is not rural AND we're applying standard 3 or 4.
        return False

def test():
    from random import randrange
    for i in range(1, 20):
        p = Params(
            pop = randrange(5000, 100000, 500),
            spd_major = randrange(35, 65, 5),
            vph_major = randrange(200, 2000, 50),
            vph_minor = randrange(20, 1000, 25),
            lanes_major = randrange(1, 4),
            lanes_minor = randrange(1, 4),
            standard = randrange(1, 4)
        )
        print "Test %d:\n" % i

        for key, value in p._asdict().iteritems():
            print "%s: %s" % (key, value)
        print "The warrant is satisfied." if is_warranted_1(p) else "The warrant is not satisfied."

        print ""

if __name__ == "__main__":
    test()

Note that I haven't touched on object oriented programming at all. Another approach would be to create a Standard class with objects that know about the relevant limits and how to check if they're satisfied.

Answer 2

lanes_maj >= lanes_min >= 2 means lanes_maj >= lanes_min and lanes_min >= 2 and it is different from lanes_maj >= 2 and lanes_min >= 2.

You should probably raise ValueError if none of the conditions met instead of returning None implicitly.

Tests should not require a human i.e., supply both input and expected output. You could use a random input if you compare two different implementations of the same function.

Don't optimize for speed unless your profiler says so.

There is no need to repeat the same code four times. You could put the limits into a nested list/tuple/namedtuple (don't repeat the same limit more more than once). To simplify warranted() function you could generate an additional data from it. Given generated data the implementation could look like:

lanes_limits = standard_limits[standard-1]
if lanes_maj == lanes_min == 1:
   limits = lanes_limits[0]
elif lanes_maj >= 2 and lanes_min == 1:
   limits = lanes_limits[1]
elif ...
# don't forget to check `if pop <= 10000 or spd_maj >= 40:` for standard 3,4
and_or = op[standard-1]
return and_or((vph_maj >= limits.a.maj and vph_min >= limits.a.min), 
              (vph_maj >= limits.b.maj and vph_min >= limits.b.min))