Significantly speed up operations that rely on WCS links #765
Conversation
astrofrog
requested review from
duytnguyendtn,
ibusko,
javerbukh and
rosteen
as code owners
|
Thanks! Do you think this will sufficiently resolve #723 ? |
|
I think so yes but it would be helpful to test that out |
|
Update 2021-08-06: As per offline discussions, I am waiting for a follow-up commit from @astrofrog before reviewing, as he is addressing some feedback from @rosteen . |
There was a problem hiding this comment.
Tested this branch (rebased on top of latest main) with the following versions:
numpy1.19.0astropy5.0.dev459+g55a5170f9bqplot0.12.30bqplot-image-gl1.4.4glue-core1.1.1.dev2+g8be22e3e from Add WCSLink.as_affine method to return affine approximation to WCSLink glue-viz/glue#2219glue-jupyter0.8.dev5+gc2416da from Expand fixed resolution buffer beyond the image boundaries to make panning seamless glue-viz/glue-jupyter#246 (rebased on top of its latestmaster)glue-astronomy0.2snakeviz2.1.0
Python 3.8 on Debian (WSL2 on Windows 10) with Jupyter Lab.
Profiled cell
As @rosteen advised, this cell to add static region was profiled as such:
import snakeviz
%load_ext snakeviz
c = SkyCoord('00h24m07.33s -71d52m50.71s')
# photutils aperture
my_aper = CircularAperture((600, 400), r=10)
my_aper_sky = SkyCircularAperture(c, 1 * u.arcsec)
# regions shape
my_reg = CirclePixelRegion(center=PixCoord(x=600, y=200), radius=20)
my_reg_sky = CircleSkyRegion(c, Angle(2, u.arcsec))
# Numpy mask
idx = (np.array([350, 350, 350, 350, 350, 350, 351, 351, 351, 351, 352, 352, 352,
352, 352, 352, 352, 352, 352, 352, 353, 353, 353, 353, 353, 353,
353, 353, 353, 353, 353, 353, 354, 354, 354, 354, 354, 354, 354,
354, 355, 355, 355, 355, 355, 355, 355, 355, 356, 356, 356, 356,
356, 356, 356, 357, 357, 358, 358]),
np.array([353, 354, 355, 356, 357, 358, 350, 352, 359, 361, 350, 352, 353,
354, 355, 356, 357, 358, 359, 361, 350, 351, 352, 353, 354, 355,
356, 357, 358, 359, 360, 361, 351, 352, 354, 355, 356, 357, 359,
360, 352, 353, 354, 355, 356, 357, 358, 359, 352, 353, 354, 355,
356, 357, 358, 353, 358, 352, 359]))
my_mask = np.zeros(data.shape, dtype=np.bool_)
my_mask[idx] = True
my_regions = {'my_aper': my_aper, 'my_aper_sky': my_aper_sky,
'my_reg': my_reg, 'my_reg_sky': my_reg_sky,
'my_mask': my_mask}
%snakeviz imviz.load_static_regions(my_regions)snakeviz screenshots
My result (agree better with @astrofrog):
@astrofrog 's result:
@rosteen 's result:
My findings
Overall, the performance is much better to the point it might be good enough for MVP. For people used to "instantaneous" feedback using other tools, might still not be good enough but maybe that is also not a realistic standard, as we're comparing apples to oranges. As Tom R pointed out, we have a web layer that the other tools do not have to worry about.
After adding the static regions, I tried normal pan/zoom on the linked images (just two of them), pan/zoom with WCS using an additional tiled viewer, blinking, center_on, and offset_to. They all worked well enough for me.
HOWEVER, there is a blocker that needs to be fixed. See the next section.
(Outdated) Blocker: This broke add_markers
With this patch, I can no longer use add_markers(). See traceback below (similar traceback when you try to add pixel coords or sky coords):
----> 5 imviz.add_markers(t_xy)
.../jdaviz/configs/imviz/helper.py in add_markers(self, table, x_colname, y_colname, skycoord_colname, use_skycoord, marker_name)
394 t_glue = Data(marker_name, **table[x_colname, y_colname])
395 jglue.data_collection[marker_name] = t_glue
--> 396 jglue.add_link(t_glue, x_colname, image, image.pixel_component_ids[1].label)
397 jglue.add_link(t_glue, y_colname, image, image.pixel_component_ids[0].label)
398
.../glue_jupyter/app.py in add_link(self, data1, attribute1, data2, attribute2)
111 att2 = data2.id[attribute2]
112 link = LinkSame(att1, att2)
--> 113 self.data_collection.add_link(link)
114
115 def set_subset_mode(self, mode):
.../glue/core/data_collection.py in add_link(self, links)
160 instances, or a :class:`~glue.core.link_helpers.LinkCollection`
161 """
--> 162 self._link_manager.add_link(links)
163
164 def remove_link(self, links):
.../glue/core/link_manager.py in add_link(self, link, update_external)
187 self._external_links.append(link)
188 if update_external:
--> 189 self.update_externally_derivable_components()
190
191 @contract(link=ComponentLink)
.../glue/core/link_manager.py in update_externally_derivable_components(self, data)
240 d = DerivedComponent(data, link)
241 comps[cid] = d
--> 242 data._set_externally_derivable_components(comps)
243
244 # Now update information about pixel-aligned data
.../glue/core/data.py in _set_externally_derivable_components(self, derivable_components)
1028 if self.hub:
1029 msg = ExternallyDerivableComponentsChangedMessage(self)
-> 1030 self.hub.broadcast(msg)
1031
1032 def _set_pixel_aligned_data(self, pixel_aligned_data):
.../glue/core/hub.py in broadcast(self, message)
213 logging.getLogger(__name__).info("Broadcasting %s", message)
214 for subscriber, handler in self._find_handlers(message):
--> 215 handler(message)
216
217 def __getstate__(self):
.../glue/viewers/common/viewer.py in _update_data(self, message)
271 else:
272 if layer_artist.layer is message.data:
--> 273 layer_artist.update()
274
275 def _update_subset(self, message):
.../glue/utils/matplotlib.py in wrapper(*args, **kwargs)
168 for backend in DEFER_DRAW_BACKENDS:
169 backend.draw_idle = DeferredMethod(backend.draw_idle)
--> 170 result = func(*args, **kwargs)
171 finally:
172 for backend in DEFER_DRAW_BACKENDS:
.../glue/viewers/image/layer_artist.py in update(self, *event)
198 ARRAY_CACHE.pop(self.state.uuid, None)
199 PIXEL_CACHE.pop(self.state.uuid, None)
--> 200 self._update_image(force=True)
201 self.redraw()
202
.../glue_jupyter/bqplot/image/layer_artist.py in _update_image(self, force, **kwargs)
123 if force or any(prop in changed for prop in ('layer', 'attribute',
124 'slices', 'x_att', 'y_att')):
--> 125 self._update_image_data()
126 force = True # make sure scaling and visual attributes are updated
127
.../glue_jupyter/bqplot/image/layer_artist.py in _update_image_data(self, *args, **kwargs)
136
137 def _update_image_data(self, *args, **kwargs):
--> 138 super()._update_image_data(*args, **kwargs)
139 # if the image data change, the contour lines are invalid
140 self._contour_line_cache.clear()
.../glue/viewers/image/layer_artist.py in _update_image_data(self)
147
148 def _update_image_data(self):
--> 149 self.composite_image.invalidate_cache()
150 self.redraw()
151
.../glue_jupyter/bqplot/image/frb_mark.py in invalidate_cache(self)
97
98 def invalidate_cache(self):
---> 99 self.update()
.../glue_jupyter/bqplot/image/frb_mark.py in update(self, *args, **kwargs)
87
88 # Get the array and assign it to the artist
---> 89 image = self.array_maker(bounds=bounds)
90 if image is not None:
91 with self.hold_sync():
.../glue/viewers/image/composite_array.py in __call__(self, bounds)
92
93 if callable(layer['array']):
---> 94 array = layer['array'](bounds=bounds)
95 else:
96 array = layer['array']
.../glue_jupyter/bqplot/image/layer_artist.py in get_image_data(self, *args, **kwargs)
62 return None
63 else:
---> 64 return super().get_image_data(*args, **kwargs)
65
66 def _update_visual_attributes(self):
.../glue/viewers/image/layer_artist.py in get_image_data(self, bounds)
136
137 try:
--> 138 image = self.state.get_sliced_data(bounds=bounds)
139 except (IncompatibleAttribute, IndexError):
140 # The following includes a call to self.clear()
.../glue/viewers/image/state.py in get_sliced_data(self, view, bounds)
450
451 if isinstance(self.layer, BaseData):
--> 452 image = self.layer.compute_fixed_resolution_buffer(full_view, target_data=self.viewer_state.reference_data,
453 target_cid=self.attribute, broadcast=False, cache_id=self.uuid)
454 else:
.../glue/core/data.py in compute_fixed_resolution_buffer(self, *args, **kwargs)
1943 def compute_fixed_resolution_buffer(self, *args, **kwargs):
1944 from .fixed_resolution_buffer import compute_fixed_resolution_buffer
-> 1945 return compute_fixed_resolution_buffer(self, *args, **kwargs)
1946
1947 # DEPRECATED
.../glue/core/fixed_resolution_buffer.py in compute_fixed_resolution_buffer(data, bounds, target_data, target_cid, subset_state, broadcast, cache_id)
130 for bound in bounds:
131 if isinstance(bound, tuple) and bound[2] < 1:
--> 132 raise ValueError("Number of steps in bounds should be >=1")
133
134 # If cache_id is specified, we keep a cached version of the resulting array
ValueError: Number of steps in bounds should be >=1
Markers linking logic was added in #699 following the canon example at https://github.com/glue-viz/glue-jupyter/blob/master/notebooks/Astronomy/W5/W5%20Tutorial.ipynb
UPDATE: I cannot reproduce this error! Not sure how I triggered it. ❗
| "wcs_links = wcs_autolink(viewer.session.data_collection)\n", | ||
| "for link in wcs_links:\n", | ||
| " exists = False\n", | ||
| " for existing_link in viewer.session.data_collection.external_links:\n", | ||
| " if isinstance(existing_link, WCSLink):\n", |
There was a problem hiding this comment.
Need to remove WCSLink from import in the cell above too.
|
p.s. @rosteen , I downgraded to |
|
I started again from a clean environment and it seems much faster now, I suspect I was missing a commit somewhere. The cell I was profiling now takes a total of 1.68 seconds: |
|
@rosteen , interesting how |
|
Re: breaking @astrofrog , maybe there is nothing to fix; not sure. I'll wait for your follow-up commit w.r.t. performance to re-review everything. |
|
@rosteen @pllim - please try this again with the developer version of glue-core and glue-jupyter, so not including glue-viz/glue-jupyter#246 for now. I found that glue-viz/glue-jupyter#246 introduces new issues so I'm going to have a think about better solutions - but region creation etc should now be a lot faster, and even panning should be less laggy even though it is still laggy. |
|
If this is already faster I will try and do a release of glue-core and glue-jupyter so we can merge this. |
|
Note to self: glue-jupyter panning lag fix deferred, so this will be faster but no seamless. |
|
I confirmed that this does still seem much faster (for everything but panning) even without glue-jupyter 246. The static region loading cell takes ~1 second for me ( However, I've encountered an annoying bug. When I load the two images in the ImvizExample notebook, the second image does not show up in the Layer menu until I blink between the two images. There may be other ways to get the second image to show up there, but simply loading the data and waiting doesn't do it. My work flow is simply to run the cells of the example notebook and then un-select and re-select both images in the Note that |
|
I can confirm what @rosteen saw w.r.t. second image not listed under Layers data until it is accessed. Instead of blinking, you can also force it to show in the drop-down by programmatically accessing it like |
There was a problem hiding this comment.
Seems much better, except the lazy loading on Layers drop-down. While minor, it can confuse and frustrate users. If you know of a simple fix, great. If not, we can open a follow-up bug report.
For reference, another snakeviz result:
- glue 1.1.1.dev13+g3719c4ff
- glue-jupyter 0.8.dev8+g296f0f2
|
p.s. I don't know how this is triggered but sometimes I see |
This comment has been minimized.
This comment has been minimized.
|
For completeness, seems fine with ASDF/GWCS data. --- a/notebooks/ImvizExample.ipynb
+++ b/notebooks/ImvizExample.ipynb
@@ -88,8 +88,8 @@
"metadata": {},
"outputs": [],
"source": [
- "acs_47tuc_1 = download_file('https://mast.stsci.edu/api/v0.1/Download/file?uri=mast:HST/product/jbqf03gjq_flc.fits', cache=True)\n",
- "acs_47tuc_2 = download_file('https://mast.stsci.edu/api/v0.1/Download/file?uri=mast:HST/product/jbqf03h1q_flc.fits', cache=True)"
+ "jwf277w = download_file('https://stsci.box.com/shared/static/iao1zxtigyrhq7k3wtu5nchrxzlhj9kv.fits', cache=True)\n",
+ "jwf444w = download_file('https://stsci.box.com/shared/static/rey83o5wq6g7qd7xym6r1jq9wlsxaqnt.fits', cache=True)"
]
},
{
@@ -108,8 +108,8 @@
"outputs": [],
"source": [
"imviz = Imviz()\n",
- "imviz.load_data(acs_47tuc_1, data_label='acs_47tuc_1')\n",
- "imviz.load_data(acs_47tuc_2, data_label='acs_47tuc_2')\n",
+ "imviz.load_data(jwf277w, data_label='JWST_F277W')\n",
+ "imviz.load_data(jwf444w, data_label='JWST_F444W')\n",
"\n",
"viewer = imviz.app.get_viewer('viewer-1')\n", |
pllim
force-pushed
the
speedup-wcs-links
branch
from
6df851a
to
5835780
Compare
|
New upstream releases have been pinned in #805 , so this is now GTG. I need this for next sprint, so I am just going to merge when tests pass. |
Codecov Report
@@ Coverage Diff @@
## main #765 +/- ##
=======================================
Coverage 67.75% 67.75%
=======================================
Files 65 65
Lines 4459 4459
=======================================
Hits 3021 3021
Misses 1438 1438
Continue to review full report at Codecov.
|
|
Thanks, Tom! |
Currently, a number of performance issues in jdaviz are due to pixel -> world -> pixel transformations between two images. Recently, this issue has become more apparent because the example data used in the ImViz notebook have distortions in the WCS.
However, in many cases - especially for small fields of view, the pixel -> world -> pixel transformations can be approximated very well by affine transformations (note that I'm not saying the pixel -> world transformations is affine, but the full pixel -> pixel transformations, especially for images with dither patterns etc.).
I've therefore added a method in glue to help find a good affine approximation to a WCSLink (glue-viz/glue#2219). Note that currently this errors if the maximum error is >1 pixel.
In combination with glue-viz/glue-jupyter#246 (which improves panning performance) this should now lead to much more responsive panning, region drawing, and blinking.
(I realised while working on this that in fact I think DS9 must do something similar which is how it manages to be so fast. I've noticed that images in DS9, when matching WCS, are often just rotated/translated but not actually distorted, so I think they are sometimes using affine approximations too.)
Please try this out, with both of the following PRs/branches:
Expand fixed resolution buffer beyond the image boundaries to make panning seamless glue-viz/glue-jupyter#246Before this can be merged (if we go ahead with it) we will need releases of glue-core and glue-jupyter and we might want to decide whether to allow the tolerance to be customized, and whether we should allow the user to turn on 'full WCS links'.
EDIT: Fix #723
Notes from @pllim
Needs:
Todo: