GIF/PPM to greyscale converter

Question

This is my take on an image → greyscale converter.

#greyscale.py -- Converts an image file to greyscale

from tkinter.filedialog import askopenfilename, asksaveasfilename
from graphics import *

def displayimage(imagefile):
    #Open image to get width and height
    image2convert = Image(Point(0, 0), imagefile)
    imgwidth = image2convert.getWidth()
    imgheight = image2convert.getHeight()

    #Set window size to image size and draw image to window
    win = GraphWin("Greyscale Converter", imgwidth, imgheight)
    image2convert.move(imgwidth/2, imgheight/2)
    image2convert.draw(win)

    return image2convert, win

def convert(image2convert, win):
    width = image2convert.getWidth()
    height = image2convert.getHeight()

    #For each row of pixels
    for pixelx in range(width):
        #For each column of pixels
        for pixely in range(height):
            print("Converted pixel: x:{0} y: {1}".format(pixelx, pixely))
            #Get Pixel Colour, calculate greyscale, set Pixel to calculated colour
            r, g, b = image2convert.getPixel(pixelx, pixely)
            brightness = int(round(0.299 * r + 0.587 * g + 0.114 * b))
            image2convert.setPixel(pixelx, pixely, color_rgb(brightness, brightness, brightness))
        #See progress for each row
        win.update()
    return image2convert

def main():
    imagefile = askopenfilename(filetypes=[("GIF-Image", ".gif"), ("PPM-Image", ".ppm")])

    image2convert, win = displayimage(imagefile)

    #convert after click
    win.getMouse()
    image2convert = convert(image2convert, win)

    #save new image
    outFile = asksaveasfilename(filetypes=[("GIF-Image", ".gif"), ("PPM-Image", ".ppm")])
    newImg = image2convert.save(outFile)

    #close file and window after click
    win.getMouse()
    image2convert.close()
    win.close()

main()

Opinions/Suggestions/Ideas? It is pretty slow, but I don't think that this approach can be much faster.

Answer 1

There are some very good things about this code.

It makes good use of library functionality, which is one of the big strengths of Python. A lot of programmers spend unnecessary amounts of time rewriting things that others have already done, so it's good to see the habit of just using things like askopenfilename as provided.

I also like the use of comments. There are two traps that people fall into with comments: one is not writing any, and the other is writing comments that just say what the code is already saying. A comment like "Update the window" before win.update() would be pointless, whereas your comment "See progress for each row" explains clearly and concisely what the intention behind that update is.

That said, I am slightly confused by two of your comments. It's almost midnight here and I may be misreading the code, but it looks to me as if the first for loop is going from left to right one column at a time, and the second for loop is going one row at a time. If so, those comments are the wrong way round. It's important to make sure that comments are kept up to date and accurate, or they can mislead the person reading the code (even if it's future you.)

You mention it running quite slowly. Now the general rule is that when something runs slowly, you use a tool called a profiler to work out which bits of the code are taking time. I haven't run one on this code, but in my experience that print line is probably taking a surprising amount of time. We always assume that text is easy for computers, which it kind of is, but if you think about it the computer has to set maybe a couple of thousand pixels to update that text for every one pixel that it's doing the greyscale calculation for. Although it's really useful to have such lines while developing something, it's usually worth taking them out once they have served their purpose.

Finally, I'll rattle through a few python best practices that are generally worth bearing in mind.

• Your function displayimage does two jobs. It first loads an image from a file, and then creates a window and puts it on the screen. Now, you have made good use of python's multi-return functionality to let you do that cleanly, but usually best practice is that every function should do one thing and do it well. I would suggest cutting that function in half: one function could take a file name and return an image, and a second function should take an image and set up a window for it.
• Python programmers usually prefer naming things with snake_case for variable names and function names, which means join lowercase words with underscores. That is purely a convention, but sticking to conventions helps to read code more fluently.
• It is generally discouraged to use import *. That is because it is not clear what comes from where. If I want to look at the documentation for, say, Image then I don't know whether to look at the graphics documentation or somewhere else. Even worse, there's a whole bunch of stuff in packages that you're not using, and it can trip you up if you don't realise it's there. For example you could imagine that your graphics module could also have a function called asksaveasfilename. (Ok, probably not that one, but it serves for an example) Then when you call asksaveasfilename you get all sorts of confusion because it could call the wrong one. Even worse, if you update your packages then graphics which didn't have asksaveasfilename before might have it now, and the behaviour of your program changes in entirely unexpected ways.

I hope this is helpful.

Josiah