OpenImageIO

[Ralf Gommers]

Hi Stefan,

2009/10/10 Stéfan van der Walt <stefan at sun.ac.za>

>
> Hey Ralf
>
> 2009/10/9 Ralf Gommers <ralf.gommers at googlemail.com>:
> >
> http://github.com/rgommers/scikits.image/blob/imgcollection/scikits/image/io/io.py
>
> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.python.org/pipermail/scikit-image/attachments/20091010/37055ca2/attachment.html>