Hey Ralf 2009/10/9 Ralf Gommers <ralf.gommers@googlemail.com>:
http://github.com/rgommers/scikits.image/blob/imgcollection/scikits/image/io...
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.
- 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. 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 example markup does not require the "::". 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 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 The numframes variable should not be exposed, since len() is already available. The string representation can also include information on the number of frames, e.g. cat.tiff [50 frames] Finally, ensure that read-only members are defined as properties: @property def filename(self): return _filename As always with review comments: they may be overly pedantic, so use what you find applicable and discard the rest. Cheers Stéfan