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

Performance improvements for CompositeArray #2343

Merged
merged 10 commits into from
Nov 1, 2022
2 changes: 1 addition & 1 deletion glue/plugins/tools/pv_slicer/qt/tests/test_pv_slicer.py
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ def test_set_cmap(self):
cm_mode = self.w.toolbar.tools['image:colormap']
act = cm_mode.menu_actions()[1]
act.trigger()
assert self.w._composite.layers['image']['color'] is act.cmap
assert self.w._composite.layers['image']['cmap'] is act.cmap

def test_double_set_image(self):
assert len(self.w._axes.images) == 1
Expand Down
89 changes: 76 additions & 13 deletions glue/viewers/image/composite_array.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,12 @@
# This artist can be used to deal with the sampling of the data as well as any
# RGB blending.

import warnings

import numpy as np

from glue.config import colormaps

from matplotlib.colors import ColorConverter, Colormap
from astropy.visualization import (LinearStretch, SqrtStretch, AsinhStretch,
LogStretch, ManualInterval, ContrastBiasStretch)
Expand All @@ -12,6 +16,8 @@

COLOR_CONVERTER = ColorConverter()

CMAP_SAMPLING = np.linspace(0, 1, 256)

STRETCHES = {
'linear': LinearStretch,
'sqrt': SqrtStretch,
Expand All @@ -30,13 +36,25 @@ def __init__(self, **kwargs):
self.layers = {}

self._first = True
self._mode = 'color'

@property
def mode(self):
return self._mode

@mode.setter
def mode(self, value):
if value not in ['color', 'colormap']:
raise ValueError("mode should be one of 'color' or 'colormap'")
self._mode = value

def allocate(self, uuid):
self.layers[uuid] = {'zorder': 0,
'visible': True,
'array': None,
'shape': None,
'color': '0.5',
'cmap': colormaps.members[0][1],
'alpha': 1,
'clim': (0, 1),
'contrast': 1,
Expand All @@ -50,6 +68,9 @@ def set(self, uuid, **kwargs):
for key, value in kwargs.items():
if key not in self.layers[uuid]:
raise KeyError("Unknown key: {0}".format(key))
elif key == 'color' and isinstance(value, Colormap):
warnings.warn('Setting colormap using "color" key is deprecated, use "cmap" instead.', UserWarning)
self.layers[uuid]['cmap'] = value
else:
self.layers[uuid][key] = value

Expand Down Expand Up @@ -80,7 +101,34 @@ def __call__(self, bounds=None):
img = None
visible_layers = 0

for uuid in sorted(self.layers, key=lambda x: self.layers[x]['zorder']):
# We first check that layers are either all colormaps or all single
# colors, and at the same time, if we use colormaps we check what
# the smallest alpha value is - if it is 1 then colormaps do not change
# transparency and this allows us to speed things up a little.

minimum_cmap_alpha = 1
if self.mode == 'colormap':
for layer in self.layers.values():
minimum_cmap_alpha = min(layer['cmap'](CMAP_SAMPLING)[:, 3].min(),
minimum_cmap_alpha)

# Get a sorted list of UUIDs with the top layers last
sorted_uuids = sorted(self.layers, key=lambda x: self.layers[x]['zorder'])

# In the case where we are dealing with colormaps, we can start from
# the last layer that has an opacity of 1 because layers below will not
# affect the output, assuming also that the colormaps do not change the
# alpha
if self.mode == 'colormap' and minimum_cmap_alpha == 1.:
Copy link
Collaborator

@dhomeier dhomeier Oct 31, 2022

Choose a reason for hiding this comment

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

Suggested change
if self.mode == 'colormap' and minimum_cmap_alpha == 1.:
if self.mode == 'colormap' and minimum_cmap_alpha == 1.:
sorted_uuids = sorted_uuids[-1:]
elif self.mode == 'colormap':

Not sure if I understand the combination of layers right, but if all of them have alpha=1, wouldn't you automatically see only the last, topmost one? Whereas for the case that some are transparent, still everything below the last opaque one could be ignored?
The first case could probably be caught even earlier by just picking the layer with highest self.layers[x]['zorder'], avoiding further creating of temporary arrays.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure if I understand the combination of layers right, but if all of them have alpha=1, wouldn't you

I probably did not, or rather mixed up layer and colormap alpha.
Still wondering if it would not suffice for the last layer of opacity 1, if that layer has minimum cmap_alpha of 1, to ignore all layers below. Perhaps a test with a colormap that has alpha < 1 would be helpful?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@astrofrog I've added a test with a faded colormap and the use of layer-minimum alpha above – not sure if it will make a relevant difference, but I can push the commit if you want to review.

Copy link
Member Author

Choose a reason for hiding this comment

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

Still wondering if it would not suffice for the last layer of opacity 1, if that layer has minimum cmap_alpha of 1, to ignore all layers below.

Is that not effectively what we are doing now?

Copy link
Collaborator

@dhomeier dhomeier Oct 31, 2022

Choose a reason for hiding this comment

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

If any layer has a cmap_alpha < 1 (minimum_cmap_alpha < 1), we're alway keeping the full sorted_uuids list. Instead testing for if layer['alpha'] * layer['cmap'](CMAP_SAMPLING)[:, 3].min() == 1 we may still truncate it below that layer (which works at least for my two-layer test).

Copy link
Collaborator

Choose a reason for hiding this comment

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

It might actually work with computing plane further down as well (does at least with that same test), but that may need some more testing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Feel free to push the commit so I can make sure I understand :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I've pushed the first change to just set sorted_uuids[start:] separately so they are easier to revert.
This is still not really testing cmap alpha values between 0 and 1.

start = 0
for i in range(len(sorted_uuids)):
layer = self.layers[sorted_uuids[i]]
if layer['visible']:
if layer['alpha'] == 1:
start = i
sorted_uuids = sorted_uuids[start:]
Copy link
Member Author

Choose a reason for hiding this comment

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

Would it maybe be more efficient to loop over the layers backwards by inverse zorder?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good point, yes we can just stop at the highest opaque layer.


for uuid in sorted_uuids:

layer = self.layers[uuid]

Expand All @@ -89,6 +137,7 @@ def __call__(self, bounds=None):

interval = ManualInterval(*layer['clim'])
contrast_bias = ContrastBiasStretch(layer['contrast'], layer['bias'])
stretch = STRETCHES[layer['stretch']]()

if callable(layer['array']):
array = layer['array'](bounds=bounds)
Expand All @@ -104,27 +153,41 @@ def __call__(self, bounds=None):
else:
scalar = False

data = STRETCHES[layer['stretch']]()(contrast_bias(interval(array)))
data = interval(array)
data = contrast_bias(data, out=data)
data = stretch(data, out=data)
data[np.isnan(data)] = 0

if isinstance(layer['color'], Colormap):
if self.mode == 'colormap':

if img is None:
img = np.ones(data.shape + (4,))

# Compute colormapped image
plane = layer['color'](data)
plane = layer['cmap'](data)

if minimum_cmap_alpha == 1.:

if layer['alpha'] == 1:
img[...] = 0
else:
plane *= layer['alpha']
img *= (1 - layer['alpha'])

else:

# Use traditional alpha compositing

alpha_plane = layer['alpha'] * plane[:, :, 3]

alpha_plane = layer['alpha'] * plane[:, :, 3]
plane[:, :, 0] = plane[:, :, 0] * alpha_plane
plane[:, :, 1] = plane[:, :, 1] * alpha_plane
plane[:, :, 2] = plane[:, :, 2] * alpha_plane
Comment on lines +176 to +178
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
plane[:, :, 0] = plane[:, :, 0] * alpha_plane
plane[:, :, 1] = plane[:, :, 1] * alpha_plane
plane[:, :, 2] = plane[:, :, 2] * alpha_plane
plane[:, :, :3] *= np.atleast3d(alpha_plane)

Probably not necessarily faster with the bit of extra vectorisation – doing the channels separately could in fact be more memory-efficient.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I think I had it separated out for performance for some reason


# Use traditional alpha compositing
plane[:, :, 0] = plane[:, :, 0] * alpha_plane
plane[:, :, 1] = plane[:, :, 1] * alpha_plane
plane[:, :, 2] = plane[:, :, 2] * alpha_plane
img[:, :, 0] *= (1 - alpha_plane)
img[:, :, 1] *= (1 - alpha_plane)
img[:, :, 2] *= (1 - alpha_plane)

img[:, :, 0] *= (1 - alpha_plane)
img[:, :, 1] *= (1 - alpha_plane)
img[:, :, 2] *= (1 - alpha_plane)
img[:, :, 3] = 1

else:
Expand Down Expand Up @@ -155,7 +218,7 @@ def __call__(self, bounds=None):
if img is None:
return None
else:
img = np.clip(img, 0, 1)
img = np.clip(img, 0, 1, out=img)

return img

Expand Down
7 changes: 4 additions & 3 deletions glue/viewers/image/layer_artist.py
Original file line number Diff line number Diff line change
Expand Up @@ -156,15 +156,16 @@ def _update_visual_attributes(self):
return

if self._viewer_state.color_mode == 'Colormaps':
color = self.state.cmap
self.composite.mode = 'colormap'
else:
color = self.state.color
self.composite.mode = 'color'

self.composite.set(self.uuid,
clim=(self.state.v_min, self.state.v_max),
visible=self.state.visible,
zorder=self.state.zorder,
color=color,
color=self.state.color,
cmap=self.state.cmap,
contrast=self.state.contrast,
bias=self.state.bias,
alpha=self.state.alpha,
Expand Down
7 changes: 4 additions & 3 deletions glue/viewers/image/python_export.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,15 +30,16 @@ def python_export_image_layer(layer, *args):
script += "composite.allocate('{0}')\n".format(layer.uuid)

if layer._viewer_state.color_mode == 'Colormaps':
color = code('plt.cm.' + layer.state.cmap.name)
script += "composite.mode = 'colormap'\n"
else:
color = layer.state.color
script += "composite.mode = 'color'\n"

options = dict(array=code('array_maker'),
clim=(layer.state.v_min, layer.state.v_max),
visible=layer.state.visible,
zorder=layer.state.zorder,
color=color,
color=layer.state.color,
cmap=code('plt.cm.' + layer.state.cmap.name),
contrast=layer.state.contrast,
bias=layer.state.bias,
alpha=layer.state.alpha,
Expand Down
6 changes: 4 additions & 2 deletions glue/viewers/image/tests/test_composite_array.py
Original file line number Diff line number Diff line change
Expand Up @@ -49,14 +49,16 @@ def test_shape_function(self):

def test_cmap_blending(self):

self.composite.mode = 'colormap'

self.composite.allocate('a')
self.composite.allocate('b')

self.composite.set('a', zorder=0, visible=True, array=self.array1,
color=cm.Blues, clim=(0, 2))
cmap=cm.Blues, clim=(0, 2))

self.composite.set('b', zorder=1, visible=True, array=self.array2,
color=cm.Reds, clim=(0, 1))
cmap=cm.Reds, clim=(0, 1))

# Determine expected result for each layer individually in the absence
# of transparency
Expand Down