13
\$\begingroup\$

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:

linked outside site, as it is over image size limit allowed by SO

full sized image

\$\endgroup\$
  • \$\begingroup\$ It is maybe a minor point, but the green is really, really unpleasant for the eyes. Since creating a beautiful picture should be a more motivating goal (and better to impress your friends) , I would consider switching colours. \$\endgroup\$ – Jishin Noben Jan 30 at 22:18
  • 1
    \$\begingroup\$ @JishinNoben Interesting. I selected this colours because I liked them, maybe I should make a small poll. But student will select her own colours, I will certainly will not try to present mine as superior :) \$\endgroup\$ – Mateusz Konieczny Jan 31 at 13:09
  • \$\begingroup\$ Hm. I wanted to award bounty immediately but it failed. Hopefully I will remember about it tomorrow. In case that someone wants to answer - codereview.stackexchange.com/questions/217549/… will be much more useful (code in this question was completely rewritten and is now in a much better shape). \$\endgroup\$ – Mateusz Konieczny Apr 16 at 11:30
6
+50
\$\begingroup\$

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 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.
    """
    # -1 necessary due to https://github.com/python-pillow/Pillow/issues/3597
    draw.rectangle((x, y), (x + size - 1, y + size - 1)), 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(hole_size, image_size, 3 * hole_size), repeat=2):
            draw_square(draw, x, y, hole_size, hole_color)
        yield carpet
        carpet = carpet.copy()
\$\endgroup\$
  • \$\begingroup\$ Thanks, this is really, really useful! I will certainly use many (or maybe even all) suggestion. "Follow the standard practice of writing if __name__ == '__main__': main()" - in this case I deliberately skipped it for now - to avoid both explaining everything at once and to avoid cargo cult code. I planned to start adding it once projects using more than one file will appear. Do you think it is a good idea? \$\endgroup\$ – Mateusz Konieczny Jan 30 at 20:54
  • 3
    \$\begingroup\$ If you want to avoid overwhelming a beginner with the magic of if __name__ == '__main__', then just call main() unconditionally, and tell them that you're doing it because it's good practice to put all code in a function. \$\endgroup\$ – 200_success Jan 30 at 20:56
  • \$\begingroup\$ "You can — and should — combine the two import statements into one, since Image and ImageDraw are both being imported from the same module." I am not sure about this one. PEP 8 allows it but is not mentioned as preferable, I am not seeing significant benefits here, neither pylint nor autopep8 proposed to change that... \$\endgroup\$ – Mateusz Konieczny Jan 31 at 21:48
  • \$\begingroup\$ Combining the imports is what I would have done, but I don't feel strongly about it, so I've removed "and should". \$\endgroup\$ – 200_success Jan 31 at 21:52
4
\$\begingroup\$

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))
\$\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.