2009/10/10 Stéfan van der Walt
<stefan@sun.ac.za>
Hey Ralf
Thanks for working on this!
> Questions:
> - do you want to keep the Image class in that form? It seems either a plain
> ndarray or ndarray + tags dict is enough.
I'd like to remove the image class entirely.
> - can I remove the EXIF stuff or move it to a subclass of Image? I don't
> think it belongs in the base Image class.
Yes, although having an EXIF reader as a separate function might be
handy! That code is BSD-licensed AFAIK.
What I added is BSD-licensed as well.
> - should imread be moved into io.py?
Let's leave it where it is for now. It is accessible as
scikits.image.io.imread, which is fine from a user perspective.
Other notes:
If you require PIL trunk, you need to check that it is available
explicitly. Also, the PIL import test is already done by imread.
OK. I'll move the import into the MultiImg class then, so ImageCollection still works if trunk is not available.
About naming: I'd prefer if we expand the names, i.e. MultiImage
instead of MultiImg. I've learnt this the hard way, but it seems I
can never remember my own shorthand :)
The reason was the Image class, which conflicted with the PIL import. That is solved now, so I'll expand all the names again.
The example markup does not require the "::".
I saw Pauli do that for examples that are not self-contained, i.e. can't be run with doctest. Alternatively I can use the #doctest +SKIP markup (ugly as well...).
The description of MultiImage could be changed to reflect what it is
storing, i.e. something like
class MultiImage(object):
"""Class for loading multi-layer images."""
When using try-except statements, keep the code snippet contained as
small as possible. In this case, there's no problem really, because
you specifically wait for an EOFError. In general, however, it's
safer to use the form:
i = 0
while True:
i += 1
try:
img.seek(i)
except EOFError:
break
return i
Sure, I'l change that.
Not sure whether you'll ever come across images without any frames
inside, but in those cases you need a return statement as well, as
above.
_getallframes can be simplified using _getframe:
frames = []
for i in range(len(self)):
frames.append(self._getframe(i))
return frames
_getframe opens and closes the file each time, so _getallframes should be a little faster. And it's still simple code, so I think it's worth the few lines of duplication.
The numframes variable should not be exposed, since len() is already available.
Sure, I'll make it private.
The string representation can also include information on the number
of frames, e.g.
cat.tiff [50 frames]
Makes sense.
Finally, ensure that read-only members are defined as properties:
@property
def filename(self):
return _filename
Sure.
As always with review comments: they may be overly pedantic, so use
what you find applicable and discard the rest.
Don't worry, I find the above very useful. Thanks for the feedback.
Cheers,
Ralf
Cheers
Stéfan