Iterator over row and columns

Question

I am writing a simple raytracer just for fun. I want to iterate over rows, and each row must be an iterator over columns. A strategy I produced is the following:

class ViewPlane(object):
 def __init__(self, resolution, pixel_size, gamma):
 self.resolution = resolution
 self.pixel_size = pixel_size
 self.gamma = gamma
 def __iter__(self):
 def genR():
 class IterRow(object):
 def __init__(self, plane, row):
 self.plane = plane
 self.row = row
 def __iter__(self):
 def genC():
 for column in xrange(self.plane.resolution[1]):
 origin = (self.plane.pixel_size * 
 (column - self.plane.resolution[0] / 2.0 + 0.5)
 , 
 self.plane.pixel_size *
 (self.row - self.plane.resolution[1] / 2.0 + 0.5)
 , 
 100.0
 )
 yield ( Ray(origin = origin, direction = (0.0,0.0,-1.0)), 
 (self.row,column)
 )
 return
 return genC()
 def __str__(self):
 return "Row iterator on row "+str(self.row)
 for row in xrange(self.resolution[0]):
 yield IterRow(self, row)
 return genR()

Briefly stated: ViewPlane contains the pixel plane. This class implements __iter__ which defines and returns a generator function. This generator function yields instances of an internal class IterRow, which in turn has a __iter__ method that returns a generator iterating over the columns.

I personally think that it's difficult to understand, so I was wondering if there's a cleaner strategy to achieve the same result.

• tshepang: This is from one who isn't seasoned... why self.resolution = resolution if resolution is already available to the whole class (among others)?
• tshepang: Can you run pylint on your code. I see for example that you are not using self.pixel_size. Or am I missing something?
• Stefano Borini: @Tshepang: I am using it
• tshepang: Do you use it is some code you are not showing?
• Stefano Borini: @Tshepang: it's right there, at the instantiation of Ray. I know, it's not very readable. I am changing it

Answer 1

class Foo:
 def __iter__(self):
 def innergen():
 yield something
 return innergen()

Is the same as:

class Foo:
 def __iter__(self):
 yield something

In other words, there is no need to define the generator, just yield the values.

Defining a class inside the method just serves to make things complicated and hard to read. Define the class outside of the method.

Having return at the end of a function doesn't do anything and makes it look like you don't know what you are doing. (I'm not saying that you don't, but whenever you include code with effect it raises a red flag. Did you think it do something?)

Unless you really need to access the data row by row I suggesting flattening the iterator.

Stylistically, I prefer to break expressions up rather then trying to split them across several lines as you've done. I think it produces less clutter.

Here is my resulting code:

class ViewPlane(object):
 def __init__(self, resolution, pixel_size, gamma):
 self.resolution = resolution
 self.pixel_size = pixel_size
 self.gamma = gamma
 def __iter__(self):
 for row in xrange(self.resolution[0]):
 for column in xrange(self.resolution[1]):
 origin_x = self.pixel_size * self.resolution[0] / 2.0 + 0.5
 origin_y = self.pixel_size * self.resolution[1] / 2.0 + 0.5
 origin = (origin_x, origin_y, 100)
 ray = Ray(origin = origin, direction = (0.0, 0.0, -1.0) )
 yield ray, (row, column)

I'm not familiar enough with RayTracers to know but I question having the iterator over ViewPlane produce Ray objects. An iterator is for producing the contents of the object. Is a really contained inside the ViewPlane? Or should the iterator produce coordinates and let the caller construct the Ray objects?

EDIT

Here is a version that doesn't flatten the iterator:

class ViewPlane(object):
 def __init__(self, resolution, pixel_size, gamma):
 self.resolution = resolution
 self.pixel_size = pixel_size
 self.gamma = gamma
 def row_iter(self, row):
 for column in xrange(self.resolution[1]):
 origin_x = self.pixel_size * self.resolution[0] / 2.0 + 0.5
 origin_y = self.pixel_size * self.resolution[1] / 2.0 + 0.5
 origin = (origin_x, origin_y, 100)
 ray = Ray(origin = origin, direction = (0.0, 0.0, -1.0) )
 yield ray, (row, column)
 def __iter__(self):
 for row in xrange(self.resolution[0]):
 yield self.row_iter(row)

• Stefano Borini: well, there are many things to be said. Thank you for your precious feedback. I'll answer you point by point.
• Stefano Borini: Defining a class inside the method just serves to make things complicated and hard to read. Define the class outside of the method.. Agreed. I wanted to push the limit with that code in order to stress my understanding of closures and the general feeling. It's not production code, it's let's see what happens code.
• Stefano Borini: Having return at the end of a function doesn't do anything and makes it look like you don't know what you are doing. That return was supposed to be a raise StopIteration, and I just didn't change it before posting. I wasn't concerned with that, but you are right that both (return or raising) are useless in that context.
• Stefano Borini: Unless you really need to access the data row by row I suggesting flattening the iterator. I need to iterate by row because I render by row, and at each completed row I refresh the GUI. It's easier to roll over the rows, and inside that, roll over the columns. I may also flatten, roll over the pixels and refresh at each new row, but as I said, I'm stressing my knowledge for fun and profit. I wouldn't write that code in production, ever.
• Winston Ewert: @Stefano Borini, why are you asking for a code review on push your limit code? The whole point of a code review is for helping make it production ready