Deprecating is_rgb()

[ankit-maverick]

I grepped for is_rgb and surprisingly didn't find it anywhere except colorconv.py and test_colorconv.py.
Currently, the condition for testing for an rgb image in is_gray() in this commit is the same as one used in is_rgb() which is a weak test in my opinion. Does anyone have better ideas for it?

[ahojnnes]

Deprecating means to preserve the current function, but raise a warning.

You can use the skimage.shared.deprecated decorator. There are a number of examples, how to use it.

[ankit-maverick]

@ahojnnes : Currently, there are six deprecated functions in skimage and all of them have a new function to manage the deprecated functionality. For this, how should I go on to write a function for checking whether an image is rgb or not? How would I differentiate between RGB and HSV images?

[ahojnnes]

Just do raise @deprecated().

is_gray could simply look like this np.squeeze(image).ndim == 2...

[jni]

On Thu, Apr 25, 2013 at 11:27 PM, Johannes Schönberger <
notifications@github.com> wrote:

is_gray could simply look like this np.squeeze(image.ndim == 2)...

This ignores the possibility of 3D grayscale data... I haven't been happy
with these functions for that reason and will have to change/remove them to
make SLIC work in 3D. (Yep that's still coming. =P) I understand where
they're needed, and I would suggest a rename to something like
"is_gray_2d".

Also, I think you mean

np.squeeze(image).ndim == 2

=)

[ahojnnes]

This ignores the possibility of 3D grayscale data... I haven't been happy
with these functions for that reason and will have to change/remove them to
make SLIC work in 3D. (Yep that's still coming. =P) I understand where
they're needed, and I would suggest a rename to something like
"is_gray_2d".

I agree. @stefanv what do you think?

Also, I think you mean

np.squeeze(image).ndim == 2

Yes… I changed that immediately after submitting the form, but emails won't update ;-)

[ankit-maverick]

@stefanv : Waiting for your thought on this.

[JDWarner]

It's a difficult problem. Simple dimensionality checks are always going to have counterexamples, particularly if we envision supporting 2d, 2d multichannel, 3d, and 3d multichannel data (to say nothing of n-D or n-D multichannel).

The paths forward for this sort of functionality are really:

• Subclass np.ndarray, adding the ability to specify number of channels, which dimension denotes them, and some type of meaning assigned to each (dimensionality again doesn't cut it). I'm very 👎 on this, but it could be done.
• Present a consistent API for users to select if their data is multichannel or not. 👍 in my opinion - and we're already doing this in select cases.
• Use magic, and tell users never to use volumetric datasets with final dimension of length 3[ or 4, or others].

segmentation.random_walker is a decent example of the second case; if multichannel data is provided, the user must indicate multichannel=True, otherwise grayscale is assumed.

In my second case there is no real place for an is_rgb algorithm. I would support deprecating it for the next release in preparation for removing it, since right now it may be misleading users (is_rgb(im) incorrectly returns True for XYZ, Lab, HSV, among other cases).

[ankit-maverick]

@JDWarner : Another way would be to look how SimpleCV handles this, though I am not sure if it would properly fit in skimage:
https://github.com/sightmachine/SimpleCV/blob/develop/SimpleCV/ImageClass.py#L22
https://github.com/sightmachine/SimpleCV/blob/develop/SimpleCV/ImageClass.py#L880

[stefanv]

Since we use pure NumPy arrays as images, we have no way to carry any meta-data. We do have the concept of an Image class, but most functions return pure NumPy arrays. Is that something we wish to change in the long run? @tonysyu @ahojnnes

[ahojnnes]

Hm, I am not sure if this is really necessary and I tend to think this complicates things. Every user of an image processing library should actually be aware of the underlying data he/she uses? Pure NumPy arrays are in my opinion the simplest and clearest way. (e.g. the novice module in another PR tries wrap arrays with Image/Pixel classes and I think it actually complicates rather simplifies things for new users... but that is my personal opinion).

To sum it up: I'm in favor of deprecating both is_gray and is_rgb and remove it in the next version. Maybe keep is_gray for internal usage in functions in _shared...

[stefanv]

I tend to agree. @ahojnnes Do you have any suggestions for improving the novice module? Those would be very welcome, since that is really a space to help teach Python, more than do image processing.

[jni]

Nobody asked me, but I strongly -1 the idea of a specific image class. I
have had enough headaches converting PIL images to numpy arrays, which is
ultimately where computing happens in Python. Having the data in this
format allows one to easily and transparently use libraries other than
scikit-image.

On Mon, Apr 29, 2013 at 3:16 AM, Stefan van der Walt <
notifications@github.com> wrote:

I tend to agree. @ahojnnes https://github.com/ahojnnes Do you have any
suggestions for improving the novice module? Those would be very welcome,
since that is really a space to help teach Python, more than do image
processing.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/532#issuecomment-17143399
.

[tonysyu]

Alright, I'm going to play devil's advocate here (I was actually against the idea until everyone else came out against it :).

The Image class we provide gracefully degrades to numpy arrays, so there's no compatibility issues there. If we added some metadata to describe the image type, we could at least check when given an image that is an instance of the Image class. That way we could intelligently deal with multi-channel images and grayscale volumes. If the meta-data is not there, just raise an error saying the image is ambiguous and suggest that the user wrap the array as an instance of Image (instead of raising an error, you could guess and warn the user). Nothing we have now would break, and we'd be able to support more use cases.

Alternatively, we could add a standard flag to functions to indicate the image type, which would serve the same purpose as the meta-data.

[tonysyu]

I should add: The benefit of suggesting that the user wrap the image in the Image class is that the image could be wrapped once and the type would propagate to later functions instead of requiring the user to specify the type each time. Of course, some functions (esp. ones outside of scikit-image) will end up stripping the type, so the user will still have to redefine the type on occasion.

We'd have to also make sure to propagate the class intelligently, but I think a lot of this could be handled by decorators (e.g. @higher_dimensional(out='same'), @higher_dimensional(out='gray')---if both multi-channel and gray inputs returns grayscale).

[jni]

On Mon, Apr 29, 2013 at 11:59 AM, Tony S Yu notifications@github.comwrote:

If the meta-data is not there, just raise an error saying the image is
ambiguous and suggest that the user wrap the array as an instance of Image(instead of raising an error, you could guess an warn).

This is exactly what I don't want. And in the case of raising, you can
bet your ass that everything I have now would break.

A library should get out of the way and let me work, not "helpfully"
suggest wrappers that are required only for (imho) unnecessary safety
checks.

An easy alternative would be to warn only in cases when the last
dimension is 3 or 4 and the user specified multi_channel=False (the
default?).

[tonysyu]

An easy alternative would be to warn only in cases when the last dimension is 3 or 4 and the user specified multi_channel=False (the default?).

I like this idea. I guess for multi_channel=True, we'd have to rely on the docstring to specify RGB, Lab, or whatever. (Or maybe multi_channel='RGB', etc.)

We would still want a convenience function that's similar to is_rgb to spit out a warning. (I assume the function would take the image and the multi_channel flag as inputs; e.g. check_multi_channel(image, multi_channel), which warns if necessary.)

What about functions that work for multi-channel images, but not > 2D image data. We'd still want a convenience function to check the shape and last dimension, just like is_rgb. Of course, it wouldn't be able to tell whether or not it's RGB, HSV, Lab, etc. or whether it's an unfortunately-shape grayscale volume, but regardless, you still might want to make this check. Would a different name for the same function suffice?

Thoughts?

[stefanv]

Here's another possibility that might help with readability:

with image as multi_channel(image):
   ...

[ankit-maverick]

For a function like is_multichannel_2d, would the following condition suffice?
min(image.shape) == 3 or min(image.shape) == 4 to take into account for RGB, HSV, RGBA. I still can't think of a way to classify an image of shape (m, n, 3) into multichannel or 3D volumetric.

[JDWarner]

Frankly, I don't think we want a replacement for this functionality right now. We seem to agree to deprecate the existing functions (not remove, but add @deprecated decorator), but I think it would be best to revisit this functionality after we decide.

If we go function-level multispectral using naked arrays, like random_walker, we don't need a replacement. If we go the other way, the inherent metadata will make these checks obsolete.

[ankit-maverick]

@JDWarner : I was thinking to deprecate is_rgb and instead return using is_multichannel_2d.

[JDWarner]

@ankit-maverick I understand, and in nearly all cases we aim to replace - not remove - functionality. However, I feel this is an exception to that rule. is_multichannel_2d would be slightly less misleading, but it remains flawed.

Once we reach consensus on the direction we want to go, we either don't need this at all or it will be available trivially from metadata.

[ahojnnes]

We are missing a test case for is_gray_2d or did I overlook it?

[stefanv]

Sorry to keep kicking this to-and-fro, but do we really want to introduce is_gray_2d, which does not really check whether the input is gray-level or not? Then, can't we simply have is_2d? And in that case, is it even worth adding the 2-liner?

[ahojnnes]

@stefanv No, I agree, we should just get rid of those functions. But without reading the above answers in-depth, I thought the compromise was to keep is_gray_2d... shame on me ;-)

[stefanv]

:) Great, then let's take them out.

[ankit-maverick]

@ahojnnes @stefanv : Can you check the latest deletions?

[ahojnnes]

@ankit-maverick We should deprecate both functions for a start and delete in the next release.

[ahojnnes]

As we have finally reached an end in this discussion, I'll merge! ;-)

Thanks @ankit-maverick for going through this!