Fdzfgy

I am teaching programming (in this case - 1 on 1 tutoring of a teenager interested in programming) and this code will be a final stage of a progression toward program generating a nice image of Sierpiński's triangle.

Any comments how this code can be improved are welcomed! But problems with unclear code that break standard practices are especially welcomed, performance issues are less important here.

from PIL import Image

from PIL import ImageDraw





def save_animated_gif(filename, images, duration):

    # done using https://pillow.readthedocs.io/en/latest/handbook/image-file-formats.html#saving

    first_image = images[0]

    other_images = images[1:]

    first_image.save(filename, save_all=True, append_images=other_images, duration=duration, loop=0)



def make_pattern(draw, x, y, section_size, remaining_levels):

    if remaining_levels <= 0:

        return

    hole_color = (5, 205, 65)

    corner = (x + section_size / 3, y + section_size / 3)

    # -1 necessary due to https://github.com/python-pillow/Pillow/issues/3597

    opposite_corner = (x + section_size * 2/3 - 1, y + section_size * 2/3 - 1)

    draw.rectangle((corner, opposite_corner), fill=hole_color)

    parts = 3

    for x_index in range(parts):

        for y_index in range(parts):

            x_anchor = x + section_size * x_index / parts

            y_anchor = y + section_size * y_index / parts

            new_size = section_size / 3

            new_levels = remaining_levels - 1

            make_pattern(draw, x_anchor, y_anchor, new_size, new_levels)





def make_carpet(levels, size):

    carpet_color = (5, 60, 20)

    carpet = Image.new("RGBA", (size, size), carpet_color)

    draw = ImageDraw.Draw(carpet)

    make_pattern(draw, 0, 0, size, levels)

    return carpet





levels = 7

size = 3**levels

carpets = 

carpets.append(make_carpet(0, size))

standard_frame_time_in_ms = 1200

durations = [standard_frame_time_in_ms / 2] # first stage visible for a short time

for i in range(levels - 1):

    carpets.append(make_carpet(i + 1, size))

    durations.append(standard_frame_time_in_ms)

durations[-1] *= 4 # final stage of animation visible for a long time



save_animated_gif("Sierpiński's carpet.gif", carpets, durations)

output:

full sized image

Algorithm

You can simplify the code and make it run faster if you construct the next-level carpet by continuing to work on the previous image (punching more holes in it), rather than starting with a blank slate every time. The code can look prettier and more Pythonic too, since the technique lets you get rid of the recursion.

Coding practices

It's a good habit to write docstrings, especially if you are using this code to teach a student!

Avoid free-floating code; all code should be in a function. Follow the standard practice of writing if __name__ == '__main__': main() at the end of the code.

You can — and should — combine the two import statements into one, since Image and ImageDraw are both being imported from the same module.

It should be easier to tell that the color triples represent dark green and light green. A comment would work. In my solution below, I've opted to use explanatory variables. Furthermore, the colors should be specified in a more obvious place, rather than buried in some obscure place in the code.

In Python, it is usually possible to find a more elegant way to building a list than by repeatedly .append()ing. Below, I construct carpets using a generator, and durations using the * operator.

You know that all of the coordinates should be integers. Use integer division (//) rather than floating-point division (/) wherever possible.

You can use itertools.product() to avoid nested for loops for x and y.

To split a list into the first element and subsequent elements, you can write first_image, *other_images = images.

Suggested solution

from itertools import product

from PIL import Image, ImageDraw



def save_animated_gif(filename, images, durations):

    """

    Save images as frames of an animated GIF.  Durations should specify the

    milliseconds to display each frame, and should be of the same length as

    images.

    """

    # https://pillow.readthedocs.io/en/latest/handbook/image-file-formats.html#saving

    first_image, *other_images = images

    first_image.save(filename, save_all=True, append_images=other_images, duration=durations, loop=0)



def punch_hole(draw, x, y, section_size, hole_color):

    """

    For a square with a corner at (x, y) and sides of length section_size,

    divide it into 9 tiles, and fill the center tile with hole_color.

    """

    corner = (x + section_size // 3, y + section_size // 3)

    # -1 necessary due to https://github.com/python-pillow/Pillow/issues/3597

    opposite_corner = (x + section_size * 2//3 - 1, y + section_size * 2//3 - 1)

    draw.rectangle((corner, opposite_corner), fill=hole_color)



def make_carpets(n, carpet_color, hole_color):

    """

    Generate n PIL Images, each of Sierpiński's carpet with increasing levels

    of detail.

    """

    image_size = 3**n

    carpet = Image.new("RGBA", (image_size, image_size), carpet_color)

    yield carpet

    for section_size in (3**i for i in range(n, 1, -1)):

        carpet = carpet.copy()

        draw = ImageDraw.Draw(carpet)

        for x, y in product(range(0, image_size, section_size), repeat=2):

            punch_hole(draw, x, y, section_size, hole_color)

        yield carpet



def main():

    N = 7

    DARK_GREEN = (5, 60, 20)

    LIGHT_GREEN = (5, 205, 65)



    carpets = make_carpets(N, carpet_color=DARK_GREEN, hole_color=LIGHT_GREEN)

    durations = [1200] * N              # 1200ms per frame, except...

    durations[0] //= 2                  # first frame is shorter

    durations[-1] *= 4                  # final frame is longer



    save_animated_gif("Sierpiński's carpet.gif", carpets, durations)



if __name__ == '__main__':

    main()

Alternative calculations

In the suggested solution above, I've written punch_hole() to be similar to your make_pattern(), in that they are both responsible for rendering a square of size section_size. However, the arithmetic can be simplified by specifying the size of the center hole instead, so that no division is necessary.

def draw_square(draw, x, y, size, color):

    """

    Fill a square with one corner at (x, y) with the specified color.

    """

    corner = (x + size, y + size)

    # -1 necessary due to https://github.com/python-pillow/Pillow/issues/3597

    opposite_corner = (x + 2 * size - 1, y + 2 * size - 1)

    draw.rectangle((corner, opposite_corner), fill=color)



def make_carpets(n, carpet_color, hole_color):

    """

    Generate n PIL Images, each of Sierpiński's carpet with increasing levels

    of detail.

    """

    image_size = 3**n

    carpet = Image.new("RGBA", (image_size, image_size), carpet_color)

    for hole_size in (3**i for i in range(n, 0, -1)):

        draw = ImageDraw.Draw(carpet)

        for x, y in product(range(0, image_size, 3 * hole_size), repeat=2):

            draw_square(draw, x, y, hole_size, hole_color)

        yield carpet

        carpet = carpet.copy()

I only have a few small suggestions:

I like to have "tweaks" that I may want to change later at the top of my file. This makes it easier to quickly alter them when playing around without needing to dig through the code. I'd move levels and standard_frame_time_in_ms to the top so they're a little more accessible. I might also change levels to n_levels or something similar to make it clearer that it's a number representing how many levels to have; not a collection of "levels".

Right now, you're partially populating durations with the halved time delay, then adding the rest in the loop. I don't see a good reason to append to durations in the loop though. The data being added to durations has nothing to do with data available within the loop.

I'd populate it before the loop. List multiplication makes this easy. The long variable names make this difficult to do succinctly unfortunately, but it can be split over two lines if need be:

durations = [standard_frame_time_in_ms // 2] + [standard_frame_time_in_ms] * (levels - 1)



durations = [standard_frame_time_in_ms // 2] + 

                [standard_frame_time_in_ms] * (levels - 1)

I also changed it to use integer division (//) since fractions of a millisecond likely aren't usable by the GIF maker anyway.

I'd stick the whole procedure in the bottom into a function:

def main():

    carpets = 

    carpets.append(make_carpet(0, size))



    durations = [standard_frame_time_in_ms / 2] # first stage visible for a short time



    for i in range(levels - 1):

        carpets.append(make_carpet(i + 1, size))

        durations.append(standard_frame_time_in_ms)



    durations[-1] *= 4 # final stage of animation visible for a long time



    save_animated_gif("Sierpiński's carpet.gif", carpets, durations)

Now, you can call main when you want it to run. Especially when developing using a REPL, having long-running top-level code can be a pain. You don't necessarily want the whole thing to run just because you loaded the file.

You have:

carpets.append(make_carpet(0, size))

then inside the loop you have:

carpets.append(make_carpet(i + 1, size))

I'm not a fan of duplication like this. There's usually a better way. It seems like you could just adjust the range bounds:

def main():

    carpets = 



    . . .



    for i in range(-1, levels - 1): # Start at -1 instead

        carpets.append(make_carpet(i + 1, size))



    . . .

This is basically just a transformation from a range to a list of carpets though. When "converting" one sequence to another, comprehensions come to mind:

carpets = [make_carpet(i + 1, size) for i in range(-1, levels - 1)]

Then, you can easily make it lazy if that proves beneficial in the future just by changing the to ():

# Now it's a generator that only produces values as requested instead of strictly

carpets = (make_carpet(i + 1, size) for i in range(-1, levels - 1))