Performance improvements for CompositeArray #2343

astrofrog · 2022-10-27T15:38:23Z

This attempts to speed up CompositeArray by using several tricks:

When using colormaps, we don't need to care about layers below the top-most layer that has alpha=1 since it will block everything below - this should speed up viewers with multiple images open.
When using colormaps and assuming the colormaps don't have any transparency, we can avoid a bunch of temporary array creations.
We make use of out= in the contrast/bias and stretch to avoid more temporary array creation.

glue/viewers/image/composite_array.py

pllim · 2022-10-27T17:02:15Z

glue/viewers/image/composite_array.py

+                if mode is None:
+                    mode = 'colormap'
+                elif mode == 'color':
+                    raise TypeError("Mixed color and colormaps in CompositeArray")


How often would someone encounter this now that it throws exception? I don't think there is a mechanism to prevent user from setting the first viewer layer to colormap but the second layer to monochromatic?

Also, are things like subsets and linked tables already excluded when this logic is invoked? I am not familiar with the internals here.

Glue doens't actually allow image viewers to have mixed layers - the mode is controlled by:

glue/glue/viewers/image/state.py

Lines 89 to 92 in e4e875a

color_mode = DDSCProperty(0, docstring='Whether each layer can have '

'its own colormap (``Colormaps``) or '

'whether each layer is assigned '

'a single color (``One color per layer``)')

In fact I should make CompositeArray have such a settable mode rather than checking it like this at drawing time - but that can be cleaned up/improved in future. For now, these exceptions should never be tripped by users and if they are it indicates a bug in the image viewers.

The speed up is looking very promising! But I am seeing this error raised when adding data to the viewer if the viewer itself is visible. Let me know if you need the full traceback or more details to reproduce.

Could you provide the full traceback?

File ~/repos/jdaviz/jdaviz/app.py:1380, in Application.set_data_visibility(self, viewer_reference, data_label, visible, replace) 1378 [data] = [x for x in self.data_collection if x.label == data_label] 1379 color_cycler = ColorCycler() -> 1380 viewer.add_data(data, percentile=95) 1382 add_data_message = AddDataMessage(data, viewer, 1383 viewer_id=viewer_id, 1384 sender=self) 1385 self.hub.broadcast(add_data_message) File /opt/miniconda3/envs/jdaviz-dev/lib/python3.8/site-packages/glue_jupyter/view.py:117, in IPyWidgetView.add_data(self, data, color, alpha, **layer_state) 113 def add_data(self, data, color=None, alpha=None, **layer_state): 115 data = validate_data_argument(self._data, data) --> 117 result = super().add_data(data) 119 if not result: 120 return File ~/repos/glue/glue/viewers/common/viewer.py:218, in Viewer.add_data(self, data) 216 with ignore_callback(layer.state, 'zorder'): 217 self._layer_artist_container.append(layer) --> 218 layer.update() 219 self.draw_legend() # need to be called here because callbacks are ignored in previous step 221 # Add existing subsets to viewer File ~/repos/glue/glue/utils/matplotlib.py:181, in defer_draw.<locals>.wrapper(*args, **kwargs) 177 @wraps(func) 178 def wrapper(*args, **kwargs): 180 if len(DEFER_DRAW_BACKENDS) == 0: --> 181 return func(*args, **kwargs) 183 # Don't recursively defer draws. We just check the first draw_idle 184 # method since all should be modified in sync. 185 if isinstance(DEFER_DRAW_BACKENDS[0].draw_idle, DeferredMethod): File ~/repos/glue/glue/viewers/image/layer_artist.py:202, in ImageLayerArtist.update(self, *event) 200 ARRAY_CACHE.pop(self.state.uuid, None) 201 PIXEL_CACHE.pop(self.state.uuid, None) --> 202 self._update_image(force=True) 203 self.redraw() File /opt/miniconda3/envs/jdaviz-dev/lib/python3.8/site-packages/glue_jupyter/bqplot/image/layer_artist.py:127, in BqplotImageLayerArtist._update_image(self, force, **kwargs) 123 changed = self.pop_changed_properties() 125 if force or any(prop in changed for prop in ('layer', 'attribute', 126 'slices', 'x_att', 'y_att')): --> 127 self._update_image_data() 128 force = True # make sure scaling and visual attributes are updated 130 if force or any(prop in changed for prop in ('v_min', 'v_max', 'contrast', 131 'bias', 'alpha', 'color_mode', 132 'cmap', 'color', 'zorder', 133 'visible', 'stretch', 134 'bitmap_visible', 'contour_visible')): File /opt/miniconda3/envs/jdaviz-dev/lib/python3.8/site-packages/glue_jupyter/bqplot/image/layer_artist.py:140, in BqplotImageLayerArtist._update_image_data(self, *args, **kwargs) 139 def _update_image_data(self, *args, **kwargs): --> 140 self.composite_image.invalidate_cache() 141 # if the image data change, the contour lines are invalid 142 self._contour_line_cache.clear() File /opt/miniconda3/envs/jdaviz-dev/lib/python3.8/site-packages/glue_jupyter/bqplot/image/frb_mark.py:107, in FRBImage.invalidate_cache(self) 106 def invalidate_cache(self): --> 107 self.update() File /opt/miniconda3/envs/jdaviz-dev/lib/python3.8/site-packages/glue_jupyter/bqplot/image/frb_mark.py:95, in FRBImage.update(self, *args, **kwargs) 92 bounds = [(ymin, ymax, ny), (xmin, xmax, nx)] 94 # Get the array and assign it to the artist ---> 95 image = self.array_maker(bounds=bounds) 96 if image is not None: 97 if image.dtype == np.dtype("float64"): File ~/repos/glue/glue/viewers/image/composite_array.py:106, in CompositeArray.__call__(self, bounds) 104 mode = 'color' 105 elif mode == 'colormap': --> 106 raise TypeError("Mixed color and colormaps in CompositeArray") 108 # Get a sorted list of UUIDs with the top layers last 109 sorted_uuids = sorted(self.layers, key=lambda x: self.layers[x]['zorder']) TypeError: Mixed color and colormaps in CompositeArray

Aww, man.

viewer.add_data(data, percentile=95) -- I added the percentile in spacetelescope/jdaviz#1386 but the add_data call has always been there.

we now pass a default color as well, but I temporarily removed it locally in case that was triggering the error, but that doesn't seem to be the case.

duytnguyendtn

Setting the contrast/bias via API in jdaviz is much improved with this PR. Tested with one image, the original performance for 200 random adjustments for contrast/bias was ~40 seconds, down to ~19 seconds with glue-viz/glue-jupyter#325, now to ~9 seconds with this PR!

I did run into an issue with two images: if I test the contrast/bias sliders and then try to blink, the first image seems to be blank for me. Steps to reproduce:

Load BOTH datasets in https://github.com/spacetelescope/jdaviz/blob/main/notebooks/ImvizDitheredExample.ipynb
Change Plot Options dataset to the second (since there are two datasets, the second layer (B) is visible on top. By default, Plot options iterates on the first dataset (A).)
Fiddle with contrast/bias values via API. I used the following loop:

def test():
    for i in range(0,100):
        val = np.random.randint(0,100)
        imviz.plugins['Plot Options']._obj.image_bias_value = val/100
        imviz.plugins['Plot Options']._obj.image_contrast_value = (1.0 - val/100)*4
%prun test()

Blink image using "B" key to show the first dataset (A) on top. The image is blank for me, even after trying to interact with the image layer via pan/zoom. Doesn't seem to redraw

astrofrog · 2022-10-27T18:37:00Z

Ah yes that should be an easy fix

astrofrog · 2022-10-27T19:32:39Z

@duytnguyendtn - could you try the blinking again?

duytnguyendtn · 2022-10-28T00:23:45Z

Yup, that fixed it! Thanks @astrofrog!

astrofrog · 2022-10-31T13:26:24Z

@kecnry - could you try the latest version of this PR along with glue-viz/glue-jupyter#331 to see if the error you were seeing goes away?

@duytnguyendtn - could you test this again as there are some non-trivial changes that I have made?

kecnry · 2022-10-31T13:34:28Z

The error does seem to have gone away, but am now seeing the deprecation warning about color vs cmap and the behavior seems to have changed with opaque layers that are set to use colormaps (it seems the layers are now "adding" instead of only showing the top layer). Is there something else on our end we need to change because of this deprecation?

astrofrog · 2022-10-31T13:35:57Z

@kecnry - just to check, are you using the PR for glue-jupyter mentioned above? If so I will double check if I missed any other calls that need to be updated.

pllim · 2022-10-31T13:38:00Z

That deprecation warning sounds familiar. Is it from matplotlib?

astrofrog · 2022-10-31T13:38:51Z

@pllim - no it is from this PR.

kecnry · 2022-10-31T13:42:47Z

Ah, something happened with the dev install and it wasn't updating. I forced to re-install and now that went away - sorry about the false alarm! The errors do go away now and everything I've quickly tested so far seems to work as expected, but I'll keep testing.

astrofrog · 2022-10-31T13:45:22Z

@kecnry - ok thanks! I will leave this open a bit and merge later today if no further issues - then will release glue-core v1.6

dhomeier · 2022-10-31T14:30:25Z

Will changing test_pv_slicer.py to use self.w._composite.layers['image']['cmap'] do for fixing the tests?

astrofrog · 2022-10-31T14:37:08Z

Yes that will probably do - would you be happy to push the change to this PR?

astrofrog · 2022-10-31T16:41:09Z

@dhomeier did you mean to push all these commits? (Np if so, just checking!)

dhomeier · 2022-10-31T16:42:22Z

Sorry for messing up the commit history; perhaps should have rebased on this branch rather than main!
I've also tested against latest glue-jupyter, maybe would be good to add a dev test suite there as well to test after merging this PR.

dhomeier · 2022-10-31T16:45:38Z

I did mean to push the 'Comment' button ;-), so bottom line I did not intend to – don't know if I could try to reset to your last commit here and restart from there, but as there are no merge conflicts, maybe everything will be fine once this is in main...

Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>

…olor mode, disallowing mixed modes

dhomeier · 2022-10-31T16:47:25Z

Oh, maybe I should have rebased on main (again) after pulling this branch.

astrofrog · 2022-10-31T17:50:16Z

Could you still do the rebase? Thanks!

dhomeier · 2022-10-31T18:07:15Z

OK, a number of warnings about skipping previously applied commits locally, but that seems to have done it.

dhomeier · 2022-10-31T19:51:35Z

glue/viewers/image/composite_array.py

+        # 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.:


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.~~

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?

@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.

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?

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).

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.

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

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.

dhomeier

With or without either of my last two commits, should be ready to merge.

dhomeier · 2022-11-01T00:43:30Z

glue/viewers/image/composite_array.py

+                    plane[:, :, 0] = plane[:, :, 0] * alpha_plane
+                    plane[:, :, 1] = plane[:, :, 1] * alpha_plane
+                    plane[:, :, 2] = plane[:, :, 2] * alpha_plane


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.

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

astrofrog

Looks good to me! See one comment below in case it would be easy for you to implement but otherwise feel free to merge.

astrofrog · 2022-11-01T09:12:41Z

glue/viewers/image/composite_array.py

+                if layer['visible']:
+                    if layer['alpha'] * layer['cmap'](CMAP_SAMPLING)[:, 3].min() == 1:
+                        start = i
+            sorted_uuids = sorted_uuids[start:]


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

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

dhomeier · 2022-11-01T17:17:27Z

I've tried @duytnguyendtn's test above, and everything looks ok there. Might be a tiny speedup (from 15 to 14 ms per CompositeArray call) from the combined last 4 or 5 commits.

astrofrog mentioned this pull request Oct 27, 2022

[BUG] Imviz: Laggy performance of Contrast/Bias sliders spacetelescope/jdaviz#1775

Closed

pllim reviewed Oct 27, 2022

View reviewed changes

duytnguyendtn reviewed Oct 27, 2022

View reviewed changes

astrofrog marked this pull request as ready for review October 28, 2022 07:50

astrofrog mentioned this pull request Oct 31, 2022

Fix compatibility of image viewer with recent versions of glue-core glue-viz/glue-jupyter#331

Merged

astrofrog and others added 6 commits October 31, 2022 17:46

Speed up CompositeArray for common use cases

f52ce3d

Improvements to implementation

e75969f

Ignore invisible layers

136fd83

Fix typo

909329c

Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>

Have a global toggle in CompositeArray to choose either colormap or c…

d23394c

…olor mode, disallowing mixed modes

Use layers[image][cmap] in TestStandaloneImageViewer

b10d464

dhomeier force-pushed the speedup-composite-array branch from 4f9a42a to b10d464 Compare October 31, 2022 18:05

dhomeier reviewed Oct 31, 2022

View reviewed changes

dhomeier added 2 commits October 31, 2022 23:35

Always start from highest fully opaque layer

02aefe0

Use simplified layer composition for local cmap_alpha==1, too

1a54498

dhomeier approved these changes Nov 1, 2022

View reviewed changes

astrofrog commented Nov 1, 2022

View reviewed changes

dhomeier added 2 commits November 1, 2022 16:59

Search for first opaque layer from top

717ea8a

TST: allow macOS pyside63 to fail

3dc2d5d

dhomeier force-pushed the speedup-composite-array branch from 3e2256c to 3dc2d5d Compare November 1, 2022 16:00

astrofrog merged commit 1e3d319 into glue-viz:main Nov 1, 2022

bmorris3 mentioned this pull request Nov 3, 2022

[BUG] Imviz: deprecation warning from glue-core v1.6 spacetelescope/jdaviz#1818

Closed

	color_mode = DDSCProperty(0, docstring='Whether each layer can have '
	'its own colormap (``Colormaps``) or '
	'whether each layer is assigned '
	'a single color (``One color per layer``)')

Performance improvements for CompositeArray #2343

Performance improvements for CompositeArray #2343

Conversation

astrofrog commented Oct 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duytnguyendtn left a comment

Choose a reason for hiding this comment

astrofrog commented Oct 27, 2022

astrofrog commented Oct 27, 2022

duytnguyendtn commented Oct 28, 2022

astrofrog commented Oct 31, 2022

kecnry commented Oct 31, 2022

astrofrog commented Oct 31, 2022

pllim commented Oct 31, 2022

astrofrog commented Oct 31, 2022

kecnry commented Oct 31, 2022

astrofrog commented Oct 31, 2022 • edited

dhomeier commented Oct 31, 2022

astrofrog commented Oct 31, 2022 • edited

astrofrog commented Oct 31, 2022

dhomeier commented Oct 31, 2022

dhomeier commented Oct 31, 2022

dhomeier commented Oct 31, 2022

astrofrog commented Oct 31, 2022

dhomeier commented Oct 31, 2022

dhomeier Oct 31, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhomeier Oct 31, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhomeier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astrofrog left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhomeier commented Nov 1, 2022 • edited

astrofrog commented Oct 31, 2022 •

edited

astrofrog commented Oct 31, 2022 •

edited

dhomeier Oct 31, 2022 •

edited

dhomeier Oct 31, 2022 •

edited

dhomeier commented Nov 1, 2022 •

edited