Writing .ppm images to a file

I am struggling with commenting and variable naming. My teacher says my comments need to be more explanatory and explicit. He says my variable names are also sometimes confusing. I was just wondering whether you could go through my code and see whether you are able to understand the comments and add in comments/edit where you feel I should add comments or improve them. Lastly, are there any general rules to follow with commenting?

class PPM(object):
    def __init__(self, infile, outfile):
        self.infile=infile
        self.outfile=outfile

        #Read in data of image
        data= open(self.infile,"r")
        datain=data.read()
        splits=datain.split(None, 4)

        #Header info and pixel info
        self.type=splits[0]
        self.columns=int(splits[1])
        self.rows=int(splits[2])
        self.colour=int(splits[3])

        #(Return a new array of bytes)
        self.pixels=bytearray(splits[4])

    def grey_scale(self):
            for row in range(self.rows):
                for column in range(self.columns):
                    start = row * self.columns * 3 + column * 3
                    end = start + 3
                    r, g, b = self.pixels[start:end]
                    brightness = int(round( (r + g + b) / 3.0 ))
                    self.pixels[start:end] = brightness, brightness, brightness

    def writetofile(self):
        dataout= open(self.outfile, "wb")
        #Use format to convert back to strings to concatenate them and Those {} in the write function get's replaced by the arguments of the format function.
        dataout.write('{}\n{} {}\n{}\n{}'.format(self.type,
                                                 self.columns, self.rows,
                                                 self.colour,
                                                 self.pixels))

sample = PPM("cake.ppm", "Replica.ppm")
sample.grey_scale()
sample.writetofile()

To steal an old quote: "There are 2 hard things in computer science. Naming, cache invalidation, and off-by-one errors".

That being said, there is room for improvement here. Firstly, I'm assuming the class name, PPM, is short for Portable Pixmap Format. However, this isn't immediately obvious, and if you aren't familiar with that format (I'm not), it required a search. Hence, the first thing I'd do is change the name to something a bit more descriptive, and add a docstring explaining something about the format:

class PortablePixmap(object):
    '''A class encapsulating basic operations on images that use the
       portable pixmap format (PPM). 

    '''

Python itself has a style guide known as PEP8 that you should try to follow as much as possible. Generally the convention in python is to name ClassesLikeThis, methods_like_this, and variables_like_this. Hence, another change I'd make is to rename infile and outfile to in_file and out_file respectively.

Continuing on, the first comment under __init__ is fairly obvious:

#Read in data of image
data= open(self.infile,"r")
datain=data.read()

As a minor aside, try and keep the whitespace around operators like = consistent. Again, as per PEP8, these should be:

data = open(self.infile, "r")
data_in = data.read()

I'd also consider renaming data_in to something like raw_image_data.

Back to the comments. The next line has no comment, but needs it far more than the previous 2 lines:

# Break up the image data into 4 segments because ...
splits = datain.split(None, 4)

The comment #(Return a new array of bytes) is both obvious and misleading: this is __init__; you're constructing the object - assigning to self.pixels isn't returning anything!

For grey_scale, your indentation moves to 8 spaces instead of 4. Be careful with this - especially in Python, where whitespace can modify the semantics (meaning) of your program. This function should again have a docstring:

def grey_scale(self):
    '''Converts the supplied image to greyscale.'''

The final function, def writetofile(self): should again use _ as separators
to make it easier to read. I'd also probably move the out_file parameter to this function, rather than passing it in __init__:

def write_to_file(self, output_location):
    '''Writes the image file to the location specified output location.
       The file is written in .

    '''

Watch the length of your comments (they should stick to the same line lengths as everything else in your program).

#Use format to convert back to strings to concatenate them and Those {} in the write function get's replaced by the arguments of the format function.

The comment itself is also difficult to understand:


"convert back to string to concatenate them and Those {} ..."

That made me do a double take. Try and write comments as complete sentences.

StackExchange Code Review Q#38963, answer score: 16