5
\$\begingroup\$

I was writing a program for my math class because my Professor asked me to. The problem is as follows: 300 people are arranged in 30 rows each of 10 people. The tallest of each row is chosen, and the shortest of each column. The problem is to prove whether the shortest of the taller people is taller, or the tallest of the shorter people. My program is as follows, but 90% of my coding standards are from PPCG, so my code is likely awful.

import random

multiple = int(input("Would you like multiple runs or one (enter 1 or 0)?\n").rstrip())

def weighted_random():
    # between 3 ft and 6 ft
    temp = random.random()
    t = 0
    if temp > 0.95:
        t = 6
    elif temp < 0.1:
        t = 3
    elif temp < 0.5:
        t = 4
    else:
        t = 5
    return t + random.random()

def output_array(array):
    for i in array:
        for j in i:
            print("%.3f" % j, end=' ')
        print()

def _zip(array):
    _x = len(array[1])
    _temp = []
    for i in range(_x):
        _temp.append([j[i] for j in array])
    return _temp

heights = [[weighted_random() for j in range(10)] for i in range(30)]

if not multiple:
    print("Men")

    output_array(heights)

    tall = [max(i) for i in heights]

    tall_man = min(tall)

    short = [min(i) for i in _zip(heights)]

    short_man = max(short)

    print("Tallest of short: ", end='')

    print(short_man)

    print("Shortest of tall: ", end='')

    print(tall_man)

    print("Answer is: ", end='')

    print(["tallest of short", "shortest of tall"][short_man < tall_man])

else:
    evalf = []

    times = int(input("Number of evaluations?\n").rstrip()) or 10000

    zeroes = 0

    for _i in range(times):
        heights = [[weighted_random() for j in range(10)] for i in range(30)]

        tall = [max(i) for i in heights]

        tall_man = min(tall)

        short = [min(i) for i in _zip(heights)]

        short_man = max(short)

        if short_man >= tall_man:

            zeroes += 1

    print("Out of %d evaluations, we got %d results as shortest of tall taller than tallest of short, and %d times they were equal." % (times, times - zeroes, zeroes))
\$\endgroup\$
4
\$\begingroup\$

An old question, bet let's leave no question unanswered... I will go through your code (mostly) in order in which it is presented.

Asking for multiple is, in my opinion, in the wrong position, as it is separated by functions definitions from the rest of the main code. I'd move it after the functions definitions.

Closely related to that, asking the user to answer with C-like logic of 1 for "yes" or 0 for "no" is not pleasant, especially since Python doesn't share C's woes with strings. Btw, int(...) has no problem with extra spaces, so (r)stripping is not needed.

And, since we're becoming user-friendly here, let's allow users to make typos without crashes and other unexpected behaviour by asking them again if they give a nonsense answer. All together:

while True:
    answer = input("Would you like answer runs (y/n)? ").strip().lower()
    if answer in {"y", "n", "yes", "no"}:
        break
multiple = (answer in {"y", "yes"})

Creating weighted randoms can be done in a more concise and configurable way. I have chosen this:

weights = [0, 0, 0, 0.1, 0.5, 0.95, 1]
def weighted_random():
    # between 3 ft and 6 ft
    major = random.random()
    major = next(idx for idx,limit in enumerate(weights) if limit > major)
    return major + random.random()

The weights list (could've been a tuple as well) contains limits. The major value (the integer part) of the height is the index of the first value bigger than the random that was picked. So, for example, 0.05 is between 0 and 0.1, meaning that we pick major = 3 because 3 is the index of 0.1.

Apart from keeping the code short, this makes it trivial to change the weights, including adding more weight categories, as long as we keep to the premise that these define the integer part. Otherwise, we could've used a dictionary or some similar approach.

We create heights twice in the program, so let's not copy/paste this non-trivial and important code. A function is better:

def make_heights(rows=30, cols=30):
    return [[weighted_random() for j in range(cols)] for i in range(rows)]

Printing is also best done using generators and joining the elements with the space character:

def output_heights(data):
    print("Men:")
    for row in data:
        print(" ".join("%.3f" % el for el in row))

Your _zip function confused me at first. There is a zip() in Python, but it does something far more reasonable for a function with such a name. Consider renaming this to "transpose".

Either way, I think it's best to handle "the shortest of the tall ones" and "the tallest of the short ones" the same way, with a single function for each of them:

def shortest_of_talls(data):
    return min(max(row) for row in data)

def tallest_of_shorts(data):
    return max(min(row[j] for row in data) for j,_ in enumerate(data[0]))

Here, I use enumerate to get (index, value) tuples, and I use a throwaway variable _ for values because I don't need them. There are a few situations when _ is not a good choice, but that's beyond the scope of this review.

Btw, notice that I use data. It's more descriptive than array (albeit, heights or people might have been a better choice).

Your main if looks like this: if not multiple: ... else: .... Does it not make more sense to do if multiple: ... else: ...? I changed it thus, so let's first review the "multiple" code.

I prefer evalf = list() over evalf = [], as I find it more readable. You're more likely to notice a bug if you put set() instead of list() than if you put {} instead of [].

We decided above to be more user friendly, so let's keep that up when asking about the number of runs. You wanted to ask the user to give you an integer, and default to 10000 if they give 0. Here is the code that takes an empty(ish) string (we allow spaces) which is treated as the default, or a positive integer. Any other response will repeat the question:

    while True:
        times = input("Number of evaluations (leave empty for 10000)? ")
        if times.strip() == "":
            times = 10000
            break
        try:
            times = int(times)
            if times > 0:
                break
        except ValueError:
            pass

The next part of your code is biased toward what you know (expect?) is the right answer. If I got it right, this is a research question, so let's research the results properly, by counting how many "tallest of shorts", "shortest of talls", and "equals" are there:

    cnt_tos = cnt_sot = cnt_eq = 0

Since your default is to run 10000 tests, and that takes time (roughly a minute on my laptop), I decided to add progress information here. It's a neat little trick: print a message, but end with "\r" instead of "\n" to place the cursor at the beginning of the same line, instead of the next one:

    cnt_tos = cnt_sot = cnt_eq = 0
    done = -1
    for run in range(times):
        new_done = 100*run // times
        if new_done > done:
            done = new_done
            print("Done: %d%%" % done, end="\r")
        heights = make_heights()

        tall_man = shortest_of_talls(heights)
        short_man = tallest_of_shorts(heights)

        if tall_man > short_man:
            cnt_sot += 1
        elif tall_man < short_man:
            cnt_tos += 1
        else:
            cnt_eq += 1

    print("Out of %d evaluations, we got:" % times)
    print("  * {times} times ({percent:.3f}%) the shortest of tall was taller,".format(times=cnt_sot, percent=100*cnt_sot/times))
    print("  * {times} times ({percent:.3f}%) the tallest or short was taller,".format(times=cnt_tos, percent=100*cnt_tos/times))
    print("  * {times} times ({percent:.3f}%) they were equal.".format(times=cnt_eq, percent=100*cnt_eq/times))

Notice that I have removed many blank lines that you have. The Style Guide for Python, which is one of the core documents for Python programmers, suggests that those should be used sparingly. I am probably too generous with them, but I prefer separating parts of the code that do semantically different parts of the job.

I also replaced C-style formats for the newer format() function, to make the code more readable (and, in theory, easier to change/translate, as these arguments are no longer positional). I will rarely use C-style formatting if there is more than one variable involved.

I did similar changes to your "single run" branch:

    heights = make_heights()
    output_heights(heights)

    tall_man = shortest_of_talls(heights)
    short_man = tallest_of_shorts(heights)

    print("Tallest of short:", short_man)
    print("Shortest of tall:", tall_man)
    print(
        "The answer is: ",
        "tallest of short" if short_man > tall_man else "shortest of tall"
    )

As you can see, Python has ternary operator, and this is far more readable than the original print(["tallest of short", "shortest of tall"][short_man < tall_man]).

Here is my variant of your code as a whole:

#!/usr/bin/env python3

import random

# Between 3 ft and 6 ft
weights = [0, 0, 0, 0.1, 0.5, 0.95, 1]

def weighted_random():
    major = random.random()
    major = next(idx for idx,limit in enumerate(weights) if limit > major)
    return major + random.random()

def make_heights(rows=30, cols=30):
    return [[weighted_random() for j in range(cols)] for i in range(rows)]

def output_heights(data):
    print("Men:")
    for row in data:
        print(" ".join("%.3f" % el for el in row))

def shortest_of_talls(data):
    return min(max(row) for row in data)

def tallest_of_shorts(data):
    return max(min(row[j] for row in data) for j,_ in enumerate(data[0]))

while True:
    answer = input("Would you like answer runs (y/n)? ").strip().lower()
    if answer in {"y", "n", "yes", "no"}:
        break
multiple = (answer in {"y", "yes"})

if multiple:
    evalf = list()
    while True:
        times = input("Number of evaluations (leave empty for 10000)? ")
        if times.strip() == "":
            times = 10000
            break
        try:
            times = int(times)
            if times > 0:
                break
        except ValueError:
            pass

    cnt_tos = cnt_sot = cnt_eq = 0
    done = -1
    for run in range(times):
        new_done = 100*run // times
        if new_done > done:
            done = new_done
            print("Done: %d%%" % done, end="\r")
        heights = make_heights()

        tall_man = shortest_of_talls(heights)
        short_man = tallest_of_shorts(heights)

        if tall_man > short_man:
            cnt_sot += 1
        elif tall_man < short_man:
            cnt_tos += 1
        else:
            cnt_eq += 1

    print("Out of %d evaluations, we got:" % times)
    print("  * {times} times ({percent:.3f}%) the shortest of tall was taller,".format(times=cnt_sot, percent=100*cnt_sot/times))
    print("  * {times} times ({percent:.3f}%) the tallest or short was taller,".format(times=cnt_tos, percent=100*cnt_tos/times))
    print("  * {times} times ({percent:.3f}%) they were equal.".format(times=cnt_eq, percent=100*cnt_eq/times))
else:
    heights = make_heights()

    output_heights(heights)

    tall_man = shortest_of_talls(heights)
    short_man = tallest_of_shorts(heights)

    print("Tallest of short:", short_man)
    print("Shortest of tall:", tall_man)
    print(
        "The answer is: ",
        "tallest of short" if short_man > tall_man else "shortest of tall"
    )

Apart from these, I have three more comments:

  • Use sensible names. Apart from zip and array, there was also zeroes, which was probably meant as "those where the condition is zero (it's actually False in Python)", but this is really not informative to anyone reading the code.

  • Consider including docstrings in all of your functions. It's a good habit to have, lest you end up banging your head on the desk when reading trying to understand your own code in a few months.

  • Since you're working with matrices, using NumPy's array could be a good choice, especially because it provides a simple and fast transpose and weighted sampling.

\$\endgroup\$
2
\$\begingroup\$

I'm just going to take this top to bottom.

First of all, you should use if __name__ == '__main__' to hold all of your things that aren't going to be exported functions/classes/variables. This is best practice, easiest to read, and nicest to would-be importers.

Next, your weighted_random function is too hard-coded. You should use a list of operators and thresholds. Then it is easy to test this, change it, etc. Note that this may not be the best solution if you end up getting a lot of different choices, at it requires looping over all of them.

import operator    

height_weights = [
    (operator.gt, 0.95, 6),
    (operator.lt, 0.1, 3),
    (operator.lt, 0.5, 4)
]

def weighted_random(weights, default):
    rand_val = random.random()
    result = default
    for op, threshold, res in weights:
        if op(rand_val, threshold):
            result = res
    return result + random.random()

Your output_array should probably be called display_matrix, and you should use the more modern format. It would also be much cleaner to use str.join. Add some comprehensions, and you'll have something that looks like this:

def display_matrix(matrix):
    print(
        '\n'.join(
            ' '.join("{:.3f}".format(column) for column in row)
            for row in matrix
        )
    )

_zip is a pretty bad name for this next function. What you're really doing is rotating the matrix so it is column-major instead of row-major. I'd write something like this

def rotate(matrix):
    return [
        [row[i] for row in matrix]
        for i in range(len(matrix[1]))
    ]

Next I'd write functions like get_tallest and get_shortest, as well as get_heights, etc. The full change is here

import random
import operator


height_weights = [
    (operator.gt, 0.95, 6),
    (operator.lt, 0.1, 3),
    (operator.lt, 0.5, 4)
]

def weighted_random(weights, default):
    rand_val = random.random()
    result = default
    for op, threshold, res in weights:
        if op(rand_val, threshold):
            result = res
    return result + random.random()

def display_matrix(matrix):
    print(
        '\n'.join(
            ' '.join("{:.3f}".format(column) for column in row)
            for row in matrix
        )
    )

def rotate(matrix):
    return [
        [row[i] for row in matrix]
        for i in range(len(matrix[1]))
    ]

def get_tallest(heights):   
    tall = [max(i) for i in heights]
    tall_man = min(tall)
    return tall_man

def get_shortest(heights):
    short = [min(i) for i in rotate(heights)]
    short_man = max(short)
    return short_man

def short_is_taller(heights):
    return get_shortest(heights) >= get_tallest(heights)

def get_heights(rows=30, columns=10):
    return [[weighted_random(height_weights, 5) for _ in range(columns)] for _ in range(rows)]

if __name__ == '__main__':
    multiple = int(input("Would you like multiple runs or one (enter 1 or 0)?\n").strip())
    heights = get_heights()

    if not multiple:
        print("Men")
        display_matrix(heights)

        tall_man = get_tallest(heights)
        short_man = get_shortest(heights)

        lines_to_print = (
            "Tallest of short: {}".format(short_man),
            "Shortest of tall: {}".format(tall_man),
            "Answer is: {}".format(["tallest of short", "shortest of tall"][short_man < tall_man])
        )

        print('\n'.join(lines_to_print))
    else:
        times = int(input("Number of evaluations?\n").rstrip()) or 10000

        times_short_is_taller = sum(short_is_taller(get_heights()) for i in range(times))

        print("Out of {} evaluations, we got {} results as shortest of tall taller than tallest of short" % (times, times_short_is_taller))
\$\endgroup\$

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.