6
\$\begingroup\$

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.

\$\endgroup\$
7
  • \$\begingroup\$ This is from one who isn't seasoned... why self.resolution = resolution if resolution is already available to the whole class (among others)? \$\endgroup\$ Commented May 16, 2011 at 5:31
  • \$\begingroup\$ Can you run pylint on your code. I see for example that you are not using self.pixel_size. Or am I missing something? \$\endgroup\$ Commented May 16, 2011 at 5:33
  • \$\begingroup\$ @Tshepang: I am using it \$\endgroup\$ Commented May 16, 2011 at 7:06
  • \$\begingroup\$ Do you use it is some code you are not showing? \$\endgroup\$ Commented May 16, 2011 at 12:16
  • \$\begingroup\$ @Tshepang: it's right there, at the instantiation of Ray. I know, it's not very readable. I am changing it \$\endgroup\$ Commented May 16, 2011 at 13:07

1 Answer 1

Reset to default
4
\$\begingroup\$ 
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)
\$\endgroup\$
12
  • \$\begingroup\$ well, there are many things to be said. Thank you for your precious feedback. I'll answer you point by point. \$\endgroup\$ Commented May 16, 2011 at 18:30
  • \$\begingroup\$ 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. \$\endgroup\$ Commented May 16, 2011 at 18:32
  • \$\begingroup\$ 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. \$\endgroup\$ Commented May 16, 2011 at 18:34
  • \$\begingroup\$ 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. \$\endgroup\$ Commented May 16, 2011 at 18:37
  • \$\begingroup\$ @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 \$\endgroup\$ Commented May 16, 2011 at 18:38

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.