search

pyapp-kit / ndv

Simple, fast-loading, n-dimensional array viewer with minimal dependencies.
BSD 3-Clause "New" or "Revised" License
40 stars 7 forks source link

feat: Use handles to compute viewer range #38

Closed gselzer closed 2 months ago

gselzer commented 3 months ago

The bug that I describe in https://github.com/gselzer/pymmcore-plus-sandbox/issues/15 actually originates from the reusage of PImageHandles within this project. This PR provides one simple solution for solving this issue, however I'm not sure that it is the best solution, hence the draft status.

The Problem:

Suppose you start up an NDViewer instance with data of shape (512, 512), and after viewing it you then call viewer.setData(new_data) where new_data.shape is (256, 256). My concrete use case here is

  1. Use NDViewer to display a snap returned by pymmcore-plus
  2. Change the binning size of my Camera device to 2, halving the size of images returned
  3. Use the same NDViewer to display a new snap

The image now appears within the top left quadrant of the canvas, which I think is not necessarily bad, however if you then click the "Set Range" button, the range does not change, and the image stays within the top left quadrant.

Under the Hood

The issue stems from a couple different shortcuts taken within ndv:

  1. Within NDViewer._update_canvas_data, handles are reused when available. Therefore, we only create a handle for the (512, 512) image, and reuse it for the (256, 256) image.
  2. Within VispyViewerCanvas.set_range, the range itself comes from the self._current_shape variable, which is only updated when a new handle is created. Thus, when we reuse the handle for the new data, this member of the canvas is not updated.

Note that self._current_shape variable does not accommodate multiple PImageHandle objects on the same canvas either.

The Solution:

Since I'm assuming it won't be terribly expensive to recompute the total shape every time we press the button, we might as well try it, but the question is where that computation should occur. It could either occur within the PCanvas.set_range function, implemented by each backend, or it could occur within NDViewer._on_set_range_clicked. I started out implementing the function in the latter as it covers both backends simultaneously, however it's probably smarter to fix this within each backend, especially since both have a self._elements dictionary whose values should contain all handles. The other downside is that more work would be required to ensure that only active handles are involved in the computation, as I do not believe that stale handles are purged from either dictionary upon removal.

Happy to hear your thoughts @tlambert03

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 92.85714% with 2 lines in your changes missing coverage. Please review.

Project coverage is 77.43%. Comparing base (3ee756c) to head (bcf41e9). Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/ndv/viewer/_backends/_vispy.py 92.85% 2 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #38 +/- ## ========================================== + Coverage 77.33% 77.43% +0.09% ========================================== Files 13 13 Lines 1827 1839 +12 ========================================== + Hits 1413 1424 +11 - Misses 414 415 +1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

gselzer commented 3 months ago

Re-implemented this calculation on the backend