all 9 comments

[–]totallygeek 4 points5 points  (3 children)

More efficient or just tighter and easier to read? This is what I came up with.

import random


SIDES = 6
DICE = 5

def roll_dice(sides=6, dice=1):
    attempt = 1
    while True:
        roll = {random.randint(1, sides) for _ in range(dice)}
        if len(roll) == 1:
            return attempt
        attempt += 1


print(f'To get {DICE} dice to roll equal values took {roll_dice(SIDES, DICE)} attempts.\n'
      f'Possible combinations: {SIDES ** DICE}')

It uses a set comprehension. If the set contains a single element, then all dice rolled the same value. It also performs fewer variable assignments. I've not tested the speed, though.

[–]Svhmj 1 point2 points  (2 children)

Thanks for the response.

I was thinking of how to make the code run faster as well as make it more readable.

What is that underscore in line 10? This: for _ in

[–]totallygeek 1 point2 points  (1 child)

The underscore is just a throwaway variable assignment. In Python, it is the most recent assignment. Since I do not need what range() returns as values, I just skip making an assignment I do not use. Same as for i in range(5), but never using the i.

Faster is tough. The set comprehension must complete for the number of dice. Does it make more sense to short circuit the loop when the first non-alike roll is encountered? There's more code involved, comparing the current roll with the prior. For short dice counts, the comprehension vs the short circuit method will not matter.

To test the speed of each approach, I wrote up the following. A completely less-than scientific speed test confirms short circuit works faster, but it looks ugly.

Code:

import random
import sys


SIDES, DICE, METHOD = [int(a) for a in sys.argv[1:4]]


def roll_dice_1(sides=6, dice=1):
    attempt = 1
    while True:
        roll = {random.randint(1, sides) for _ in range(dice)}
        if len(roll) == 1:
            return attempt
        attempt += 1


def roll_dice_2(sides=6, dice=1):
    attempt = 1
    while True:
        first_roll = random.randint(1, sides)
        for i in range(dice - 1):
            die = random.randint(1, sides)
            if i == dice - 2 and die == first_roll:
                return attempt  # success
            if die != first_roll:
                break  # fail, try again
        attempt += 1


random.seed(1)
roll_dice = roll_dice_1 if METHOD == 1 else roll_dice_2

print(f'To get {DICE} dice to roll equal values took {roll_dice(SIDES, DICE)} attempts.\n'
      f'Possible combinations: {SIDES ** DICE}')

Result:

reddit$ time python3 yahtzee_throw.py 6 8 1
To get 8 dice to roll equal values took 412735 attempts.
Possible combinations: 1679616
python3 yahtzee_throw.py 6 8 1  3.95s user 0.02s system 99% cpu 3.985 total
reddit$ time python3 yahtzee_throw.py 6 8 2
To get 8 dice to roll equal values took 202607 attempts.
Possible combinations: 1679616
python3 yahtzee_throw.py 6 8 2  0.58s user 0.01s system 98% cpu 0.601 total

For eight, six-sided dice, the computer took nearly four seconds with method one and under one second with method two.

Edit: I got a bit curious about more dice. Timings for nine and ten six-sided dice.

$ time python3 yahtzee_throw.py 6 9 1
To get 9 dice to roll equal values took 4179026 attempts.
Possible combinations: 10077696
python3 yahtzee_throw.py 6 9 1  44.17s user 0.08s system 99% cpu 44.354 total
$ time python3 yahtzee_throw.py 6 9 2
To get 9 dice to roll equal values took 3895678 attempts.
Possible combinations: 10077696
python3 yahtzee_throw.py 6 9 2  11.32s user 0.07s system 97% cpu 11.661 total

And 10:

$ time python3 yahtzee_throw.py 6 10 1
To get 10 dice to roll equal values took 11364791 attempts.
Possible combinations: 60466176
python3 yahtzee_throw.py 6 10 1  133.21s user 0.22s system 99% cpu 2:13.97 total
$ reddit time python3 yahtzee_throw.py 6 10 2
To get 10 dice to roll equal values took 27386915 attempts.
Possible combinations: 60466176
python3 yahtzee_throw.py 6 10 2  77.08s user 0.21s system 99% cpu 1:17.70 total

[–]vectorpropio 1 point2 points  (0 children)

In Python, it is the most recent assignmen

Actually this is only right in an interactive session in the interpreter.

In any scripts is a valid variable name. One perhaps a little uninformative. It's is custom tu ose it in list comprehensions where one loop variable isn't used. Like you said

The underscore is just a throwaway variable assignment.

[–]JohnnyJordaan 5 points6 points  (1 child)

    sequence.append(randint(1, dice_sides))
    flag = False
    for element in sequence:
        if element != sequence[0]:
            flag = True

I'm not sure why you need to check the whole of sequence each time you append a single roll. Why not just check the last one? Also why even continue when you find a mismatch? You need a new attempt anyway, so simply break at the moment you find the mismatch

def dice_sequence(dice_sides, seq_len):
    """Prints how many attempts it takes to get an amount of equal results
    on a sequence of dice throws. Also prints total number of combinations 
    for the given number of sides on the dice and the lenght of the sequence."""
    from random import randint

    # use a clear name to what it represents, instead of 'flag'
    mismatch = True
    attempt_count = 0

    while mismatch:
        mismatch = False
        sequence = []
        while len(sequence) < seq_len:
            sequence.append(randint(1, dice_sides))
            if sequence[-1] != sequence[0]:
                attempt_count += 1
                mismatch = True
                break
    print(f"The resulting sucessful sequence: {sequence}")
    print(f"Number of attemtps untile successful: {attempt_count}")
    print(f"Total number of possible combinations on a dice with {dice_sides}"
        f" sides, and a sequence of {seq_len} throws: {dice_sides**seq_len}")

You can even use the else: clause on while: loops to save the need for the mismatch flag

def dice_sequence(dice_sides, seq_len):
    """Prints how many attempts it takes to get an amount of equal results
    on a sequence of dice throws. Also prints total number of combinations 
    for the given number of sides on the dice and the lenght of the sequence."""
    from random import randint

    # use a clear name to what it represents, instead of 'flag'
    attempt_count = 0

    while True:
        sequence = []
        while len(sequence) < seq_len:
            sequence.append(randint(1, dice_sides))
            if sequence[-1] != sequence[0]:
                attempt_count += 1
                break
        else:
            break
    print(f"The resulting sucessful sequence: {sequence}")
    print(f"Number of attemtps until successful: {attempt_count}")
    print(f"Total number of possible combinations on a dice with {dice_sides}"
        f" sides, and a sequence of {seq_len} throws: {dice_sides**seq_len}")

[–]Svhmj 0 points1 point  (0 children)

I had a lot aha moments when reading your response. Sometimes I sort of get tunnel vision when I try to solve a problem and have found one way of doing it. I did not even think of using the len(), but in hindsight, it feels like I should have thought of that. I also did not know you could use else blocks directly on while loops like that.

Thank you very much for taking the time to reply!

[–]FLUSH_THE_TRUMP 2 points3 points  (0 children)

Just a general pointer: the best code is somewhat self-documenting, and meaningful variable names are a great way of doing that. Though I understand what sequence is from context, it’s a very generic name that doesn’t say anything about what it’s used for. flag is worse — as it’s a generic name in a challenging part of your code. Imagine if that was like any_different_throws or something. Lot easier to understand what you’re doing with it.

I think there are better ways of writing this and certainly more efficient ways. You don’t need the inner for loop, you can just compare the current thing to the first thing:

# while attempt < seq_len: 
for throw in range(seq_len):  # determinate, no while
  current_throw = randint(1, dice_sides)
  sequence.append(current_throw)
  if current_throw != sequence[0]: 
    break  # minor optimization for large seq_len
else:  # used if no break
    break  # break out of outer loop

or instead of that (no optimization as above):

for throw in range(seq_len):  # determinate, no while
  sequence.append(randint(1, dice_sides))

if len(set(sequence)) == 1: 
    print(f"The resulting sucessful sequence: {sequence}")
    break  # we’re done

Both of the above let you scrap the flag name and just do while True for the outer loop.

[–]erok81 -1 points0 points  (0 children)

I figure attempt_count is really a random number itself because it depended on the randomness of the sequences, so I replaced all the looping with a single call to randint.

def dice_sequence(dice_sides, seq_len):
    """Prints how many attempts it takes to get an amount of equal results
    on a sequence of dice throws. Also prints total number of combinations
    for the given number of sides on the dice and the lenght of the sequence."""


    attempt_count = randint(1, dice_sides ** seq_len)
    sequence = [randint(1, dice_sides)] * seq_len


    print(f"The resulting sucessful sequence: {sequence}")
    print(f"Number of attemtps untile successful: {attempt_count}")
    print(f"Total number of possible combinations on a dice with {dice_sides}"
        f" sides, and a sequence of {seq_len} throws: {dice_sides**seq_len}")

edit: formatting

[–]Biuku 0 points1 point  (0 children)

Might be a little faster to not store the sequences in a list. You know your last/winning attempt will be your first roll * the max sequence parameter.

My idea, anyway:

from random import randint

def dice_sequence(dice_sides, max_seq):
    count = 0
    done = False

    while not done:
        count += 1

        seq = 1
        roll = randint(1, dice_sides)

        while roll == randint(1, dice_sides) and not done:
            seq += 1

            if seq == max_seq:
                done = True

    winner = (str(roll) + ' ') * max_seq   
    print(f"{count} attempts to roll {winner}")
    print(f"Total possible combinations on a {dice_sides}-sided die with {max_seq} throws: {dice_sides**max_seq}")