patternpythonMinor
Matrix of heights test
Viewed 0 times
heightsmatrixtest
Problem
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 = 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))
Solution
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
Closely related to that, asking the user to answer with C-like logic of
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:
Creating weighted randoms can be done in a more concise and configurable way. I have chosen this:
The
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
Printing is also best done using generators and joining the elements with the space character:
Your
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:
Here, I use
Btw, notice that I use
Your main
I prefer
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:
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:
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
```
cnt_tos = cnt_sot = cnt_eq = 0
done = -1
for run in range(times):
new_done = 100*run // times
if
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:
passThe 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 = 0Since 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
Code Snippets
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"})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()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]))Context
StackExchange Code Review Q#129144, answer score: 4
Revisions (0)
No revisions yet.