Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Derive baseclass from imageio #273

Draft
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

caspervdw
Copy link
Member

This is still work in progress. To make use of the imageio plugin architecture, I derived a new baseclass called PimsFormat and implemented TiffStack_tifffile based on that as an example.

To save some work, I dropped the as_gray, dtype and process_func kwargs. Also I renamed the pixel_type to dtype and I added the shape property.

It still has the N-dimensional functionality and the lazy slicing, as well as the fancy IPython display. The reader is accessible using imageio.get_reader(<filename>, 'TIFF_tifffile')

@caspervdw
Copy link
Member Author

caspervdw commented Jul 21, 2017

I fixed the tests fails that appeared due to an added dependency to Pillow via imageio. I explicitly added this dependency (along with jinja2 in the testing environment).

Via imageio, PIMS picks up two additional required deps: Pillow and olefile.

edit: I'll push the commits on Monday

@caspervdw caspervdw force-pushed the imageio-baseclass branch 2 times, most recently from c14f6f1 to d76002b Compare July 24, 2017 07:25
@tacaswell
Copy link
Member

Interesting.

I am 👍 on going this route. From my point of view the interesting thing about pims is the lazy fancy slicing. If we can solve the file dispatch / loading problem by becoming an imageio plugin all the better!

@caspervdw
Copy link
Member Author

I agree that we should take this route @tacaswell. I am however not completely content with the implementation in this PR, as it involves a custom imageio-like baseclass, which might be sensitive to future changes in imageio.

I thought of another route: if we would reimplement all our readers as imageio plugins, we'd lose all the fancy/lazy slicing capabilities. To recover that functionality, we want to wrap the imageio readers, something like:

pims.open = lambda x: WrapImageIOReader(imageio.get_reader(x))

However, we will lose knowledge of frame shape, pixel dtypes, and dimension names as they are not supported by default in imageio. These can however be guessed by reading the first frame, which is actually what happens already in many of our readers.

Preferably, we could give WrapImageIOReader all these properties, and if they do not exist in the underlying pims (or imageio) plugin, they will be investigated automatically by reading the first frame. In this way, we can give our pims plugins the frame_shape etc. attributes, while they are not strictly necessary enabling the use of non-pims imageio plugins.

Either way, I am in favour of only making one wrapper that is dimensionality-aware, based on FramesSequenceND.

If we decide on something, we should discuss with someone at imageio.

@tacaswell
Copy link
Member

That suggestion is moving the readers from being 'is-a' to 'has-a' in relation to the 'get-me-one-and-only-one' frame readers at the bottom which is also a step forward (I think).

There is a bunch of complexity in PIMS because I was thinking about streaming data from day-job related things, but that can probably all be dropped with no loss of actually use 😈 (as we do not actually use it in that way).

@danielballan
Copy link
Member

👍

@caspervdw
Copy link
Member Author

@danielballan @tacaswell I implemented the imageio plugin based architecture as described above. The implementation is surprisingly simple!

In this example, FormatTiffStack_tifffile (see here) is solely based on the imageio architecture. This reader is also accessible via imageio.get_reader and all other convenience function.

Now, WrapImageIOReader (see here) transforms this reader back into the well known FramesSequenceND subclass with all functionality conserved. It reads the first frame for shape/dtype, guesses the meaning of all dimensions, and constructs the dimension-aware FramesSequence.

For the more advanced readers of PIMS that are already aware of the dimensions (for example, Bioformats), we have to pass this information through imageio, so that the reader can be constructed. I am thinking of a method _get_pims_info that returns a dict with the necessary elements (dtype, frame_shape, sizes, ...) elements. This is already partially implemented. This would btw provide a more easy way to make any reader dimension-aware: just add a _get_pims_info method to the reader and populate the fields.

Thoughts?

@@ -22,7 +22,8 @@
cmdclass=versioneer.get_cmdclass(),
description="Python Image Sequence",
author="PIMS Contributors",
install_requires=['slicerator>=0.9.7', 'six>=1.8', 'numpy>=1.7'],
install_requires=['slicerator>=0.9.7', 'six>=1.8', 'numpy>=1.7',
'pillow', 'imageio'],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 for making imageio a hard dependency. I see that it, in turn, only requires numpy and pillow as required deps: https://github.com/imageio/imageio/blob/master/setup.py#L212

# TODO pass the dimension-awareness through this field
try:
info = self.rdr._get_pims_info()
if not isinstance(info, dict):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I follow this. Why wouldn't info be a dict?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what I was thinking, I will remove it when implementing the wrapping of readers that are already ND

@danielballan
Copy link
Member

I like the encapsulation approach. I left a comment and a question inline.

@caspervdw
Copy link
Member Author

I just pushed the rebase of a dimension aware reader (Bioformats) onto imageio. It seems to be rather straightforward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants