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

WIP: Refactor PV slicing tool #1663

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

Conversation

astrofrog
Copy link
Member

@astrofrog astrofrog commented Apr 15, 2018

Still very early days... Remaining things to do in this PR:

  • Rebase once Fix compatibility with PySide2 and drop support for PySide and PyQt4 #1662 is merged
  • Extract slice for all attributes from current reference data
  • Automatically open an image viewer if needed
  • Add status bar documentation
  • Rename tool to not be PV-specific
  • Add tests
  • Add changelog entry
  • Fix the fact that rectangular selections are somehow propagated to PV slices
  • Make subsets work properly
  • When closing window with a slice and then updating the slice, do we re-open a new window?
  • How do we deal with multiple datasets being present? do we extract all datasets as attributes in the new PV slice at resolution of reference data?
  • Seeing C/C++ reference error when closing a PV slice window and then re-openng it with data and using the mouse over path
  • Use APE 14 WCS to get nice labels on sliced data
  • Don't show PV sliced data in link editor? (since links are implicit). Or show them with a link.
  • Test that things work fine in a multi-attribute dataset

Things that could be done in future:

  • Extract slice for all visible layers (although only pixel-aligned ones)
  • Extract slice for reprojected data
  • Preserve derived components
  • Make PVSliceData components be dynamically evaluated
  • Allow thick slices
  • Have a way of selecting whether a new slice is created or modifying the old one (for now we can just have one path per viewer and have that always update)

Other thoughts

  • For now, for simplicity, don't use links between PV sliced data and parent data - allow subsets on main data to be viewer in slice but selections in slice shouldn't propagate back (except maybe via the non-slice axis)

@codecov
Copy link

codecov bot commented Jan 31, 2020

Codecov Report

Merging #1663 (e1a520b) into main (2d64631) will decrease coverage by 0.42%.
The diff coverage is 69.69%.

❗ Current head e1a520b differs from pull request most recent head a61a71c. Consider uploading reports for the commit a61a71c to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1663      +/-   ##
==========================================
- Coverage   88.07%   87.64%   -0.43%     
==========================================
  Files         247      247              
  Lines       23279    23272       -7     
==========================================
- Hits        20502    20397     -105     
- Misses       2777     2875      +98     
Impacted Files Coverage Δ
glue/core/data.py 91.66% <ø> (ø)
glue/viewers/image/qt/__init__.py 100.00% <ø> (ø)
glue/app/qt/application.py 78.38% <50.00%> (-0.19%) ⬇️
glue/plugins/tools/pv_slicer/pv_sliced_data.py 64.95% <64.95%> (ø)
glue/plugins/tools/pv_slicer/qt/pv_slicer.py 75.80% <70.96%> (+2.47%) ⬆️
glue/viewers/matplotlib/toolbar_mode.py 95.69% <87.50%> (-0.42%) ⬇️
glue/plugins/tools/pv_slicer/qt/__init__.py 100.00% <100.00%> (ø)
glue/viewers/common/qt/toolbar.py 95.07% <100.00%> (-2.81%) ⬇️
glue/viewers/image/qt/data_viewer.py 100.00% <100.00%> (ø)
glue/viewers/image/qt/profile_viewer_tool.py 33.33% <100.00%> (+9.92%) ⬆️
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2d64631...a61a71c. Read the comment docs.

@astrofrog
Copy link
Member Author

I'm currently stuck on an issue with Matplotlib getting stuck in an infinite loop in the following case:

  • Set up an RGB image with three cubes
  • Extract a path
  • Update the path

When the path is updated, the second viewer then gets stuck in a drawing loop and never renders fully. Some things that seem to matter - if in the state class for the image viewer I return a random array instead of the fixed resolution buffer after the initial call, then there is no infinite loop. If I do the same in the PV SlicedData, it sometimes helps and sometimes not (randomly). This makes me think there is a race condition of some kind.

@astrofrog
Copy link
Member Author

astrofrog commented Feb 10, 2022

Actually I think I found where the issue comes from - if I change the get_image_data on the layer artist to comment out the attribute disabling (which triggers a clear):

    def get_image_data(self, bounds=None):

        if self.uuid is None:
            return None

        # try:
        image = self.state.get_sliced_data(bounds=bounds)
        # except (IncompatibleAttribute, IndexError):
        #     # The following includes a call to self.clear()
        #     # self.disable_invalid_attributes(self.state.attribute)
        #     return None
        # else:
        #     self.enable()

then I get the following error which shouldn't be happening:

Traceback (most recent call last):
  File "/home/tom/Code/matplotlib/lib/matplotlib/backends/backend_qt.py", line 465, in _draw_idle
    self.draw()
  File "/home/tom/Code/glue/glue/viewers/matplotlib/qt/widget.py", line 124, in draw
    return super(MplCanvas, self).draw(*args, **kwargs)
  File "/home/tom/Code/matplotlib/lib/matplotlib/backends/backend_agg.py", line 436, in draw
    self.figure.draw(self.renderer)
  File "/home/tom/Code/matplotlib/lib/matplotlib/artist.py", line 73, in draw_wrapper
    result = draw(artist, renderer, *args, **kwargs)
  File "/home/tom/Code/matplotlib/lib/matplotlib/artist.py", line 50, in draw_wrapper
    return draw(artist, renderer)
  File "/home/tom/Code/matplotlib/lib/matplotlib/figure.py", line 2859, in draw
    mimage._draw_list_compositing_images(
  File "/home/tom/Code/matplotlib/lib/matplotlib/image.py", line 133, in _draw_list_compositing_images
    a.draw(renderer)
  File "/home/tom/python/dev/lib/python3.9/site-packages/astropy/visualization/wcsaxes/core.py", line 464, in draw
    super().draw(renderer, **kwargs)
  File "/home/tom/Code/matplotlib/lib/matplotlib/artist.py", line 50, in draw_wrapper
    return draw(artist, renderer)
  File "/home/tom/Code/matplotlib/lib/matplotlib/axes/_base.py", line 3105, in draw
    mimage._draw_list_compositing_images(
  File "/home/tom/Code/matplotlib/lib/matplotlib/image.py", line 133, in _draw_list_compositing_images
    a.draw(renderer)
  File "/home/tom/Code/matplotlib/lib/matplotlib/artist.py", line 50, in draw_wrapper
    return draw(artist, renderer)
  File "/home/tom/Code/matplotlib/lib/matplotlib/image.py", line 636, in draw
    im, l, b, trans = self.make_image(
  File "/home/tom/Code/mpl-scatter-density/mpl_scatter_density/base_image_artist.py", line 186, in make_image
    array = self._array_func(bins=bins, range=((ymin, ymax), (xmin, xmax)))
  File "/home/tom/Code/glue/glue/viewers/image/frb_artist.py", line 27, in array_func_wrapper
    array = self._array_maker(bounds)
  File "/home/tom/Code/glue/glue/viewers/image/composite_array.py", line 94, in __call__
    array = layer['array'](bounds=bounds)
  File "/home/tom/Code/glue/glue/viewers/image/layer_artist.py", line 138, in get_image_data
    image = self.state.get_sliced_data(bounds=bounds)
  File "/home/tom/Code/glue/glue/viewers/image/state.py", line 452, in get_sliced_data
    image = self.layer.compute_fixed_resolution_buffer(full_view, target_data=self.viewer_state.reference_data,
  File "/home/tom/Code/glue/glue/plugins/tools/pv_slicer/pv_sliced_data.py", line 211, in compute_fixed_resolution_buffer
    result = compute_fixed_resolution_buffer(self.original_data, new_bounds,
  File "/home/tom/Code/glue/glue/core/fixed_resolution_buffer.py", line 244, in compute_fixed_resolution_buffer
    if target_cid is not None and isinstance(data, Data) and isinstance(data.get_component(target_cid), DaskComponent):
  File "/home/tom/Code/glue/glue/core/data.py", line 1434, in get_component
    raise IncompatibleAttribute(component_id)
glue.core.exceptions.IncompatibleAttribute: PRIMARY

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant