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

SKIP: The way `skimage` processes and stores metadata #2605

Open
soupault opened this issue Apr 10, 2017 · 8 comments

Comments

@soupault
Copy link
Member

commented Apr 10, 2017

Description

So far the typical pipeline of working with images in skimage looks as follows:

image_file       -0->
io.imread        -1->
utils.img_as_... -2->
image_proc       -3->
image_proc       -4->
io.imsave        -5->
image_file

In the past months we had a lot of discussions and issues connected to the way we handle image metadata. To be concrete (considering the steps above):

  1. We lose and, therefore, do not exploit the metadata contained in image file. Ref: #2513 (comment), #2513 (comment), #2175 (comment), #1940, #1940 (comment), #1995;
  2. We have issues on converting from one format to another due to a lack of knowledge on initial image limits. Plus, we lose the information on precision loss, etc during the conversion step. Sometimes this leads to pretty weird warnings. Ref: #2215;
  3. The current convention is that an image is represented by a numpy array, and each function operates on it independently without knowing what this array actually is, and what operations had already been performed before. We have issues guessing if the input image is multichannel (only several of the functions support multichannel keyword), what colorspace is it of (remember, that multichannel covers mostly RGB as it is). This also leads to an issue of chaining functions - #1334, #1529. Ref: #2175, #2240 (comment), #2240 (comment), #2240 (comment), #2240 (comment);
  4. Same as 3.;
  5. And, of course, we cannot write the complete metainformation to an output file. So, basically, we have rich DICOM-like file in -> numpy array out.

I hope that the above ensures you that metainformation is really important, and most of the functions can greatly benefit from using it. Of course, there is a lot of metainfo entries, and we are not obliged to take care of all of them. What we should do, in my opinion, is to have a framework for handling the metadata, and a way to easily extend this support.

What we have now: See the snippet above: some of the functions have multichannel kw, we guess a lot on if the image is multichannel, we assume everything is RGB or grayscale, we check occasionally if input float images are in [-1; 1] (sometimes we don't), same for int images, etc.

What we could have: A metadata passed to all the relevant functions via either (a) a number of keywords (multichannel, colorspace, names of the channels (for multispectral data), isotropic/anisotropic data), or (b) as a dictionary (dictionary can store all of the fields plus the processing history).

(a) approach is quite dirty, hardly extendable (a lot of deprecation pain), but easy.

(b) approach is tidy, easily extendable, but complicates a bit the usage in a mixed (along with other libraries) environment. Allows to store and pass through the history of image processing. Allows to pass through non-handled metainfo.

@scikit-image/core I'd like to ask your opinions on this topic. I'm personally in favour of approach (b) despite the fact that it implies some breaking changes. In any case, the goal here is to work out a common solution, and make the library more convenient for all users.

@blink1073

This comment has been minimized.

Copy link
Member

commented Apr 10, 2017

A third option is what imageio does: add a .meta attribute to the numpy array with the metadata from the file: http://imageio.readthedocs.io/en/latest/userapi.html#imageio.imread.

@jni

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2017

@soupault what a great, great issue. Thanks for compiling all the above links.

I favour option (b). I think it should be relatively straightforward to incorporate even as regards deprecations, because we can use the convention that keyword arguments override the meta dictionary, and meta can be the last kwarg to any function. (This is a common way to deal e.g. with config files, which are read in but individual values can be overwritten with command-line args.)

@blink1073 That was my original preference a couple of years ago, but I think @stefanv pointed out that it is easily lost after a single round of processing. At least with a meta kwarg, you know explicitly whether it's there or not.

A couple of more things to consider:

  • Common synonyms for similar concepts: scale, spacing, resolution, pixel<dim>, ... We should maintain a "thesaurus" that translates common image metadata values to "canonical" skimage terms, and the thesaurus should be easy to update at runtime. I think working closely with imageio is a good idea here.
  • Do we maintain common kwargs as kwargs or do we long-term (1.0) want to move everything into the meta dict? I favour the former, because many pipelines should work just fine without meta.

At any rate, I think this one of just a few critical issues to figure out for an eventual 1.0 release. Thanks again @soupault for putting it together! =)

@stefanv

This comment has been minimized.

Copy link
Member

commented Jul 15, 2017

Yes, unfortunately the .meta attribute approach is not viable (the reason why the Image class didn't work either). I'm in favor of option (b). It's a bit annoying to have to drag these two objects around all the time, but at least we're explicit in where the metadata lives.

Now, w.r.t. API: there's a subtle distinction between meta data and keyword arguments. Keywords tells the function "this is how I want to look at the data", whereas meta-data says "this is what I know about my data". So, I recommend that keywords override meta-data (and, thus, it also implies that keywords should still be included in the function signature: it tells the user which parts of meta-data are important to the function, and allows explicitly overriding behavior as described above).

@jni

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2018

Just to revive this discussion a bit: I had a couple of conversations at SciPy 2018 about this. I'll give one-sentence overviews, but hoping that the people I ping will chime in with more details about their ideas.

First I'd like to point out that we've moved to ImageIO for our IO in #3126. I don't know currently whether we toss out the input imageio.Image object and just keep the array, but either way, metadata input is now in skimage. I'm not sure about the output capabilities of ImageIO. @almarklein?

Second, @JDWarner proposed meta=[dict] and return_meta=[True/False] for returning the metadata. This is currently what I'm leaning towards myself.

But, third, as a counterpoint, @danielballan suggested that changing what is returned based on input values is a bad habit, and he's ok with scikit-image being the lowest-level layer in the image processing stack that others can build on (ie essentially the current status quo), and that if we want to support meta maybe we can do this in a parallel namespace (skimage.meta.*) that would always accept and return the metadata dictionary.

To be honest, although I am very sympathetic to the principles of type purity promoted by this proposal, the end result makes me unhappy. "Practicality beats purity", and a .meta namespace seems impractical to me. But I'm not ready to dismiss the idea outright.

The truth is that the Pythonic way to do this is indeed a more complex object that builds on NumPy arrays (e.g. XArrays), and/or duck-typing (e.g. adding a .meta attribute to whatever array-like we pass), but both of these are unpalatable for our library. I really think the main reason that skimage is where it is today is because of the simplicity and interoperability of working with NumPy arrays, and we absolutely must not jeopardise that.

@almarklein

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2018

I'm not sure about the output capabilities of ImageIO. @almarklein?

The Writer object (obtained with e.g. imageio.get_wirter(...)) has .append_data(im, meta=None). (It also checks whether im has a .meta attribute.) At the moment, imageio.imwrite() does not expose a way to provide meta data; we should probably fix that.

I wrote up some things from imageio's end here: imageio/imageio#362 In short, I'd be happy to accommodate for skimage's needs, and if possible, maybe even get rid of that Image class.

I'm looking forward to see what you'll come up with. My 2ct for a new namespace; I agree that a meta namespace alongside the current namespace would be impractical, but an "io2" namespace that will eventually replace (perhaps only by convention) the current io namespace would allow starting with a clean sheet.

@hmaarrfk

This comment has been minimized.

Copy link
Member

commented Aug 7, 2018

I'm not sure what value we can add by having yet an other custom way of handling metadata. I'm of the mindset that we should not create an other class/collection of objects that acts like Novice (even if you call it meta). Without a clear focus, we simply won't maintain it.

I also want to make sure we also don't get stuck on the "color conversion" issue too much. While it is an important issue, we should be able to TRUST the user of our library and not hand hold them too much. I agree it is the first issue people run into when they open their images for the first time, but it quickly becomes a trivial issue in my mind when users learn that their processing can probably be done on the gray image (or maybe just the red channel 🐶 ).

Keep in mind that many functions in the color conv module are limited to 10 lines of code (but 30 lines of documentation). I'm willing to bet that many users probably wrote them themselves before finding scikit-image.

Finally datatype, quantization, rounding errors, vectorization are things you MUST learn (slowly) when doing data analysis. We can't shield users from that. What do we do when the user mangles the metadata dictionary and opens an issue?

Most functions only need one or two keyword arguments regarding the image's metadata. Asking a user to buy into a whole OO system is pretty costly. It would have probably turned me away from scikit-image. That is basically what turned me away from PIL's opaque images.

I've come to use xarray quite a bit. That said, the real value is that it allows me to keep track of my experimental data and acts more like a digital notebook that can be shared with others. It isn't meant for me to "magically find the right axis where hsv is stored". To teach users about metadata mangement, we can point them to other libraries we find useful. If other strategies fall in line with the array protocol, then maybe scikit-image doesn't have to do much to accommodate users that want to use fancy numpy arrays.

At the end of the day, I think the power of scikit-image will be to position itself as a powerful library for image processing, as opposed to metadata handling. I am of the mind that documentation and keywords like axis to find where the rgb axis is, are basically all we need for now.

Regarding the strange warnings, I think those would be best addressed by raising an error. I was bit by the rank filters myself. I disagree with the "transparent" approach taken there. For those particular functions, the conversion doesn't gain any speed over img_as_float(rank.the_filter(img_as_byte(my_float_image))) and therefore I don't see a strong case for implicit type conversion.

@danielballan

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2018

At the end of the day, I think the power of scikit-image will be to position itself as a powerful library for image processing, as opposed to metadata handling. I am of the mind that documentation and keywords like axis to find where the rgb axis is, are basically all we need for now.

This is my position too. @jni accurately summarized my misgivings about breaking return type stability above. A more serious concern I have is that correctly propagating metadata through, and updating it accurately to reflect the action of a given function, is a hard problem. It also makes it harder to immediately see what a function's inputs are. If a function accepts a dictionary of metadata, how do I know which keys in that dictionary affect the function's behavior and which are just being passed through (and maybe updated)? I think it better to force the user to handle these things and pass in exactly what a function needs, so both the author and future readers of the code will understand what the dependencies are, i.e. what choices are effectively being made.

To add onto @stefanv and @jni's remarks above about a .meta attribute: we started down that path in pims with Frame.metadata and we quickly regretted it, not least of all because functions from external libraries that users might apply to their images (e.g., numpy.sum) do not know about this system and cannot participate in it.

Similarly, the suggestion of capturing the "processing history" as part of this worries me. Many projects in the workflow-management/provenance-capturing space have tried to do this with mixed results, and I think it would be risky for scikit-image to attempt to adopt this goal as part of its core functionality.

But that doesn't mean scikit-image can't do anything to help. As a concrete step, would it make sense for the reading functions to return a image, dict_of_metadata, adopting canonical skimage names in the process? This would enable users to write lossless (or less lossy) converters, to start.

@jni

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2018

Hey @danielballan, nice to see you again! =P

correctly propagating metadata through, and updating it accurately to reflect the action of a given function, is a hard problem

Yes, but I will need to solve it at some point. So the question is whether it should live in scikit-image or in various field-specific subprojects. Over on the corresponding imageio issue, I kind of argued myself into the latter corner. =P A big advantage of field-specific option is that one only needs to worry about some subset of metadata. For example, in light microscopy, one wants emission wavelengths for each channel, but these would be pretty strange to deal with in normal photographs.

the suggestion of capturing the "processing history" as part of this worries me.

Fair points. Just an idea. =) I don't necessarily mean to do something fancy. From the scikit-image perspective it could contain a list of strings containing the function name and the values of the arguments. Each function would just append itself to that list. (ie this would be intended for human consumption, not machine consumption. That's an easier problem I think.)

would it make sense for the reading functions to return a image, dict_of_metadata, adopting canonical skimage names in the process

Yes, although as I mentioned on imageio, this is probably something to do at that level, rather than skimage.

If a function accepts a dictionary of metadata, how do I know which keys in that dictionary affect the function's behavior and which are just being passed through (and maybe updated)?

I think @stefanv's distinction is very good here: let's never alter a function's behaviour based on meta. Rather, pass meta through and update if necessary, and maybe warn if a kwarg contradicts a meta parameter (such as spacing=, for example).

To teach users about metadata mangement, we can point them to other libraries we find useful.

This is a very good idea to grow from. I think if we can get imageio "standard metadata" working for a few image file types, then we can start to implement metadata handling not in code, but in documentation. As we write more docs about how to deal with metadata correctly, the right approach for code might become apparent.

I don't see a strong case for implicit type conversion.

I'm coming along to this point of view. =)

Thanks all, this is becoming a very useful thread indeed!

@jni jni changed the title SKIEP: The way `skimage` processes and stores metadata SKIP: The way `skimage` processes and stores metadata Aug 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.