-
Notifications
You must be signed in to change notification settings - Fork 29
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
New function to accept multiple images for segmentation #91
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JohnnyTeutonic have you attempted to track down the LAB range issues from imshow_rand? Otherwise, I've added some minor comments.
gala/viz.py
Outdated
@@ -55,20 +55,61 @@ def imshow_rand(im, labrandom=True): | |||
fig : plt.Figure | |||
The image shown. | |||
""" | |||
rand_colors = np.random.rand(np.ceil(im.max()), 3) | |||
rand_colors = np.random.rand(int(np.ceil(im.max())), 3) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's replace this with:
from math import ceil # <-- near top of file
[...]
rand_colors = np.random.random(size=(ceil(np.max(im)), 3))
gala/viz.py
Outdated
return plt.imshow(im, cmap=rcmap, interpolation='nearest') | ||
|
||
|
||
def multiple_images(*images, raw=False, image_type='rand'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the use case here? Show two segmentations of the same image side by side?
I actually like the basic idea for this function, but the current interface hampers its usability. For example, it might be good to display the original image as well, but that would require a different colormap...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also it needs a "show" in the name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, I've already incorporated the ability to show the original image side-by-side with the other images. The use case is to allow for the inclusion of multiple segmented images alongside an original image. This is going to back to a conversation we had a few weeks ago. I have included the parameter 'raw' which lets you get the raw versions of the images. If you want to have a randomised color image alongside the raw image, then it will do this by default for you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the colormap is the same for every displayed image, right? Whereas normally you would want grey/magma for the raw image, and randomised colours for the segmentation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well yeah my code is a bit hacky right now but I know what you are asking for. I have been trying to incorporate that suggestion today and hadn't had much luck. I'm sure I'll figure out how to do that pretty soon though as it would be neat to be able to do that.
gala/viz.py
Outdated
for i in range(1, number_of_im+1): | ||
ax = figure.add_subplot(1, number_of_im, i) | ||
if image_type == 'grey' or image_type == 'gray': | ||
imshow_grey(images[i-1]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add an axes=
kwarg to the other imshow functions so that those specific axes are used for the imshow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just noticed this: maybe iterate in range(number_of_im)
and then you just need one i+1 instead of several i-1s?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what you mean here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for i in range(1, number_of_im+1):
...
images[i-1]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I said 'I'm not sure what you mean', I was referring to what I didn't understand what you meant by adding the axes kwarg.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the looping part started like that because I mistakenly believed that a call to figure.add_subplot required indexing beginning from one, but I have changed that now...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have just committed some changes that include the axis parameter but am not sure about the implementation so will wait to hear from you I guess
gala/viz.py
Outdated
if image_type == 'grey' or image_type == 'gray': | ||
imshow_grey(images[i-1]) | ||
elif image_type == 'jet': | ||
imshow_jet(images[i-1]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can delete Jet altogether, as it is an outdated colormap. My current preference would be imshow_magma
to replace it. =)
gala/viz.py
Outdated
ax.set_title("Image no. {} with a ''{}'' colormap.". | ||
format(i, image_type)) | ||
if raw: | ||
plt.imshow(images[i-1]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use ax.imshow
instead of plt.imshow
. See Elegant SciPy for some examples (this is the interface that MPL is moving towards, and it makes more sense to use specific objects, rather than rely on some global state).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get your reasoning there :)
gala/viz.py
Outdated
format(i, image_type)) | ||
if raw: | ||
plt.imshow(images[i-1]) | ||
ax.set_title("Image no. ''{}'' .".format(i)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use Python 3.6 syntax: ax.set_title(f'Image number {i}')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
I think the build is failing because it is building with python3.5 and I have included the 3.6 formatting syntax as you have suggested. |
I had to change the looping back to 'i-1' because plt.subplot has indexing start at 1 for compatibility purposes with MATLAB :) |
@JohnnyTeutonic you can (and should) change the Python version used for testing in Re i+1, I know that subplots use that, I'm saying, make i start from 0 and then use +1 where you have to, rather than making it start from 1 and then using -1 where you have to. =) Regarding passing axes, here's what I mean: ie instead of: plt.imshow(image) do: if ax is None:
fig, ax = plt.subplots()
ax.imshow(image) or: if ax is None:
ax = plt.gca() # get current axes
ax.imshow(image) Generally, always use |
@JohnnyTeutonic regarding the lab2xyz -> xyz2rgb color conversion, I know (vaguely) that that's what's causing it, what I'm saying is, I would like you to figure out when a Lab triplet is out of range and fix it before converting to RGB. =) OR, you could manually convert lab2xyz, use np.clip to clip the negative z values, and then use xyz2rgb to get the RGB tuples back. I want to use labrandom basically always because, to my eyes at least, it produces a much more pleasing color map than random RGB. Don't you think? |
Well I tried changing it to 3.6 in the .yml file but still failed. I have tried changing it in setup.py as well to see if that helps... |
No module named cythonize??? |
sorry, no module named Cython...Very confused. |
I am wondering whether Cython 0.17 is available yet for python 3.6? |
Well I got rid of the cython error but I am still not succeeding in getting the test to pass. |
I have changed the setup.cfg file, asv.conf.json and setup.py to reflect the adoption of 3.6. I am unsure whether this is what I should have done but obviously I can rollback if need be. Looking at the Travis build, I can see that the virtual env is being created in 3.6 but the tests are being run still in 3.5. I have no idea why... |
I've tried for several hours now to solve the problem with the Z pixels. I have tried clipping as well as altering the values for 'rand_colors' to no avail. The issue to me seems to be with the code inside skimage/color/colorconv.py, Even if you perform np.clip before calling lab2xyz or lab2rgb, it will still complain about Z pixels being below 0. It's not very straightforward to determine values that will not cause Z pixels to be below 0. |
Okay, sorry for all the comments. I tweaked the colours now and the warning is gone... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JohnnyTeutonic I think we'll just need one last round to get this in! =)
Don't worry about Travis for now. Your ongoing problems had to do with Travis caching the conda environment (see the "cache" statement at the end of the yaml file). I'll have to explore ways for you to clear it during PRs (though I hope we don't have to do so again any time soon).
The current build failure is because my tree merging package viridis hasn't been uploaded for Python 3.6 for conda. I'll update that shortly.
Thanks again and sorry for the Travis confusion!
ii1 = back1[i1] if compress else i1 | ||
ii2 = back2[i2] if compress else i2 | ||
return ii1, h2g1[i1], ii2, h1g2[i2] | ||
return ii1, h1g2[i1], ii2, h2g1[i2] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Just as a warning, I'm gonna get you to explain this change to me on Tuesday, cos it's still murky in my own head. =)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to do so!
gala/viz.py
Outdated
|
||
########################### | ||
# VISUALIZATION FUNCTIONS # | ||
########################### | ||
|
||
def imshow_grey(im): | ||
def imshow_grey(im, axis=''): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, axis should default to None.
gala/viz.py
Outdated
@@ -21,11 +22,11 @@ def imshow_grey(im): | |||
fig : plt.Figure | |||
The image shown. | |||
""" | |||
return plt.imshow(im, cmap=plt.cm.gray, interpolation='nearest') | |||
return plt.imshow(im, cmap='gray', interpolation='nearest') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And then here, you should have:
if axis is None:
fig, axis = plt.subplots()
return axis.imshow(im, cmap='gray')
Incidentally, interpolation=
is no longer required, since it's the new default in MPL 2.0:
https://matplotlib.org/users/dflt_style_changes.html#interpolation
Also, plt.subplots
makes a new figure. You could also use plt.gca
(get current axes) to plot on the currently active figure by default. I'm ambivalent about which should be the default.
gala/viz.py
Outdated
|
||
|
||
def imshow_jet(im): | ||
"""Show a segmentation using a jet colormap. | ||
def imshow_magma(im, axis=''): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same thing here.
rand_colors[:, 2] = rand_colors[:, 2] * 198 - 106 | ||
rand_colors[:, 0] = rand_colors[:, 0] * 81 + 39 | ||
rand_colors[:, 1] = rand_colors[:, 1] * 185 - 86 | ||
rand_colors[:, 2] = rand_colors[:, 2] * 198 - 108 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you document where you got these values from? (ie add a comment inside the block with a link) But, thanks for fixing it!!! =) Been living with these errors for years. =D
gala/viz.py
Outdated
return plt.imshow(im, cmap=rcmap, interpolation='nearest') | ||
|
||
|
||
def show_multiple_images(*images, ax='', raw=False, image_type='rand'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, I would have axes=None
, and be able to optionally pass in a list of axes that should be of the same length as images
. Then each image would be plotted on the corresponding axis.
gala/viz.py
Outdated
try: | ||
imshow_rand(images[i]) | ||
except ValueError: | ||
ax.imshow(images[i], axis=ax) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This just needs ax.imshow
, the axis will then be correctly selected. (indeed I'm not sure but this might just fail as currently written)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Originally it failed...I will see what happens with the new code though!
gala/viz.py
Outdated
number_of_im = len(images) | ||
figure = plt.figure() | ||
for i in range(number_of_im): | ||
ax = figure.add_subplot(1, number_of_im, i+1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then, here, you can say:
ax = (figure.add_subplot(1, number_of_im, i+1) if axes is None
else axes[i])
WTF: Probably some update to networkx that broke the edge iteration. Can you update the test to:
Not due to your PR but would be nice to have the tests passing before merging. =) |
I have tried to include the changes you suggested. I am a bit confused as to how to pass in the 'axes' parameter in my own function show_multiple_images. My use case is to allow for just one call of my function to show multiple images in the same figure. I have included your changes but am not sure how to pass in the axes object inside my function. I have looked at some documentation but still am a bit confused. |
@JohnnyTeutonic the idea is that you can create the figure ahead of time: images = ...
n_images = len(images)
fig, axes = plt.subplots(1, len(images))
show_multiple_images(images, axes, 'rand') (Incidentally, if That way, you now have the flexibility to use some other grid style: images = ... # 4 images
fig, axes = plt.subplots(2, 2)
show_multiple_images(images, axes=axes.ravel(), 'rand') And, this reminded me of something. Rather than using |
I will see if i can wrap my head around it. Incidentally, ev.sorted_vi_components I think has some buggy code....I will try and show you it in person on Tuesday. It is not giving me the desired labels that I need. You should see that some of the numbers are floating poitn numbers. They definitely correspond to the entropy scores. I think I can maybe fix the code up but only if you think I am right about this... |
btw let's keep the discussion on here related to the PR. Also, try to use gist.github.com to share notebooks, rather than screengrabs. You can upload a full notebook to a private gist and then share the URL. |
I read the docstrings of course but I found it a bit difficult to follow what was going on I guess, owing to the complexity of the format. Anyway, I get now that the first tuple is the labels, second is entropy, third is other labels, fourth is other entropy. all good. |
Here's the NumPy Docstring standard format, which you should be familiar with. =) https://github.com/numpy/numpy/blob/master/doc/HOWTO_DOCUMENT.rst.txt |
I have included a new function to accept multiple images in viz. This function accepts multiple images in its function call to allow for a convenient display of various segmented images in one figure. It also includes an optional parameter which allows for the displaying of the images in different colormaps.
I have also included a cast to int inside of imshow_rand when the random colors are being displayed, as I noticed that there was an issue with calling this function (owing to the use of np.ceil).