refactor: Pass LUTModel to LUTView (#87)

* Convert LUTView to Model-View

* rename views

* Remove set_colormap_without_signal usage in tests

* Sync autoscale and clims

* ImageHandle.cmap -> ImageHandle.colormap

Aligns with LutView

* PyGFXImageHandle.set_gamma: Allow gamma=1

* Clean up LutView

Use Optional"[LutModel]" over "LutModel | None" for Python 3.9

Remove unused ctor

* Comment _channel_controller.py

* More cleanups

* Add tests for QLutView

* Fix qt check

* Fix pytest qt skip

:)

* Add wx to dev extra

* Add LutView tests for Jupyter, Wx

* Improve wx tests

* Move autoscale clims to controller

Since the actual value to set here is gathered from the data, not from
the user, it shouldn't really be on the view. Also will make things much
easier when multiple handles have different data.

* Fix _CmapCombo.setCurrentColormap

* Test enabling autoscaling

* Fixes

* Fix tests

* use qtbot to manage QLutView

* Test controller autoscaling logic

* Make test smaller

The use of a controller was causing failures in some places.

* Ad __eq__ for ClimPolicy impls

Will prevent a fair number of signal reemissions. Thanks to @tlambert03
for the suggestion

* Clean up jupyter lutview

Thanks again @tlambert03 for the suggestion

* Clean up qt lutview

* LutView: Strengthen method descriptions

* vispy: remove fps printout

* Cleanup

* Align src and test package names

* Add note for QLutView.set_clims

* Remove breakpoint()

* Remove unused ChannelController.clims

---------

Co-authored-by: Talley Lambert <[email protected]>

Author: gselzer

pyproject.toml

@@ -86,7 +86,7 @@ docs = [
     "imageio[tifffile]",
 ]
 dev = [
-    "ndv[test,vispy,pygfx,pyqt,jupyter]",
+    "ndv[test,vispy,pygfx,pyqt,jupyter,wxpython]",
     "pytest-qt",
     "ipython",
     "mypy",

src/ndv/controllers/_array_viewer.py

@@ -192,10 +192,10 @@ def _add_histogram(self) -> None:
         histogram_cls = _app.get_histogram_canvas_class()  # will raise if not supported
         self._histogram = histogram_cls()
         self._view.add_histogram(self._histogram.frontend_widget())
-        for view in self._lut_controllers.values():
-            view.add_lut_view(self._histogram)
+        for ctrl in self._lut_controllers.values():
+            ctrl.add_lut_view(self._histogram)
             # FIXME: hack
-            if handles := view.handles:
+            if handles := ctrl.handles:
                 data = handles[0].data()
                 counts, edges = _calc_hist_bins(data)
                 self._histogram.set_data(counts, edges)
@@ -259,7 +259,7 @@ def _fully_synchronize_view(self) -> None:
             self._clear_canvas()
             self._request_data()
             for lut_ctr in self._lut_controllers.values():
-                lut_ctr._update_view_from_model()
+                lut_ctr.synchronize()
             self._update_hist_domain_for_dtype()
 
     def _on_model_visible_axes_changed(self) -> None:
@@ -427,7 +427,7 @@ def _on_data_response_ready(self, future: Future[DataResponse]) -> None:
                     lut_views.append(self._histogram)
                 self._lut_controllers[key] = lut_ctrl = ChannelController(
                     key=key,
-                    model=model,
+                    lut_model=model,
                     views=lut_views,
                 )
 

src/ndv/controllers/_channel_controller.py

@@ -3,12 +3,9 @@
 from contextlib import suppress
 from typing import TYPE_CHECKING
 
-from ndv.models._lut_model import ClimsManual, ClimsMinMax, ClimsType
-
 if TYPE_CHECKING:
     from collections.abc import Iterable, Sequence
 
-    import cmap
     import numpy as np
 
     from ndv.models._lut_model import LUTModel
@@ -27,113 +24,33 @@ class ChannelController:
     that displays the data, all for a single "channel" extracted from the data.
     """
 
-    def __init__(self, key: LutKey, model: LUTModel, views: Sequence[LutView]) -> None:
+    def __init__(
+        self, key: LutKey, lut_model: LUTModel, views: Sequence[LutView]
+    ) -> None:
         self.key = key
         self.lut_views: list[LutView] = []
-        self.lut_model = model
+        self.lut_model = lut_model
+        self.lut_model.events.clims.connect(self._auto_scale)
         self.handles: list[ImageHandle] = []
 
         for v in views:
             self.add_lut_view(v)
 
-        # connect model changes to view callbacks that update the view
-        self.lut_model.events.cmap.connect(self._on_model_cmap_changed)
-        self.lut_model.events.clims.connect(self._on_model_clims_changed)
-        self.lut_model.events.visible.connect(self._on_model_visible_changed)
-        self.lut_model.events.gamma.connect(self._on_model_gamma_changed)
-
     def add_lut_view(self, view: LutView) -> None:
         """Add a LUT view to the controller."""
+        view.model = self.lut_model
         self.lut_views.append(view)
-        # connect view changes to controller callbacks that update the model
-        view.visibilityChanged.connect(self._on_view_lut_visible_changed)
-        view.autoscaleChanged.connect(self._on_view_lut_autoscale_changed)
-        view.cmapChanged.connect(self._on_view_lut_cmap_changed)
-        view.climsChanged.connect(self._on_view_lut_clims_changed)
-        view.gammaChanged.connect(self._on_view_lut_gamma_changed)
-        self._update_view_from_model(view)
-
-    def _on_model_clims_changed(self, clims: ClimsType) -> None:
-        """The contrast limits in the model have changed."""
-        is_autoscale = not clims.is_manual
-        for handle in self.handles:
-            min_max = clims.calc_clims(handle.data())
-            handle.set_clims(min_max)
-        for v in self.lut_views:
-            v.set_clims_without_signal(min_max)
-            v.set_auto_scale_without_signal(is_autoscale)
-
-    def _on_model_gamma_changed(self, gamma: float) -> None:
-        """The gamma value in the model has changed."""
-        for view in self.lut_views:
-            view.set_gamma_without_signal(gamma)
-        for handle in self.handles:
-            handle.set_gamma(gamma)
-
-    def _on_model_cmap_changed(self, cmap: cmap.Colormap) -> None:
-        """The colormap in the model has changed."""
-        for view in self.lut_views:
-            view.set_colormap_without_signal(cmap)
-        for handle in self.handles:
-            handle.set_cmap(cmap)
-
-    def _on_model_visible_changed(self, visible: bool) -> None:
-        """The visibility in the model has changed."""
-        for view in self.lut_views:
-            view.set_channel_visible_without_signal(visible)
-        for handle in self.handles:
-            handle.set_visible(visible)
-
-    def _update_view_from_model(self, *views: LutView) -> None:
-        """Make sure the view matches the model."""
+        # TODO: Could probably reuse cached clims
+        self._auto_scale()
+
+    def synchronize(self, *views: LutView) -> None:
+        """Aligns all views against the backing model."""
         _views: Iterable[LutView] = views or self.lut_views
+        name = str(self.key) if self.key is not None else ""
         for view in _views:
-            view.set_colormap_without_signal(self.lut_model.cmap)
-            if self.lut_model.clims and (clims := self.lut_model.clims.cached_clims):
-                view.set_clims_without_signal(clims)
-
-            is_autoscale = not self.lut_model.clims.is_manual
-            view.set_auto_scale_without_signal(is_autoscale)
-            view.set_channel_visible_without_signal(True)
-            name = str(self.key) if self.key is not None else ""
+            view.synchronize()
             view.set_channel_name(name)
 
-    def _on_view_lut_visible_changed(self, visible: bool, key: LutKey = None) -> None:
-        """The visibility checkbox in the LUT widget has changed."""
-        for handle in self.handles:
-            previous = handle.visible()
-            handle.set_visible(visible)
-            if previous != visible:
-                self._update_clims(handle)
-
-    def _on_view_lut_autoscale_changed(
-        self, autoscale: bool, key: LutKey = None
-    ) -> None:
-        """The autoscale checkbox in the LUT widget has changed."""
-        if autoscale:
-            self.lut_model.clims = ClimsMinMax()
-        elif cached := self.lut_model.clims.cached_clims:
-            self.lut_model.clims = ClimsManual(min=cached[0], max=cached[1])
-
-        for view in self.lut_views:
-            view.set_auto_scale_without_signal(autoscale)
-
-    def _on_view_lut_cmap_changed(
-        self, cmap: cmap.Colormap, key: LutKey = None
-    ) -> None:
-        """The colormap in the LUT widget has changed."""
-        for handle in self.handles:
-            handle.set_cmap(cmap)  # actually apply it to the Image texture
-            self.lut_model.cmap = cmap  # update the model as well
-
-    def _on_view_lut_clims_changed(self, clims: tuple[float, float]) -> None:
-        """The contrast limits slider in the LUT widget has changed."""
-        self.lut_model.clims = ClimsManual(min=clims[0], max=clims[1])
-
-    def _on_view_lut_gamma_changed(self, gamma: float) -> None:
-        """The gamma slider in the LUT widget has changed."""
-        self.lut_model.gamma = gamma
-
     def update_texture_data(self, data: np.ndarray) -> None:
         """Update the data in the image handle."""
         # WIP:
@@ -143,20 +60,12 @@ def update_texture_data(self, data: np.ndarray) -> None:
             return
         handle = handles[0]
         handle.set_data(data)
-        if handle.visible():
-            self._update_clims(handle)
+        self._auto_scale()
 
     def add_handle(self, handle: ImageHandle) -> None:
         """Add an image texture handle to the controller."""
         self.handles.append(handle)
-        handle.set_cmap(self.lut_model.cmap)
-        self._update_clims(handle)
-
-    def _update_clims(self, handle: ImageHandle) -> None:
-        min_max = self.lut_model.clims.calc_clims(handle.data())
-        handle.set_clims(min_max)
-        for view in self.lut_views:
-            view.set_clims_without_signal(min_max)
+        self.add_lut_view(handle)
 
     def get_value_at_index(self, idx: tuple[int, ...]) -> float | None:
         """Get the value of the data at the given index."""
@@ -174,3 +83,15 @@ def get_value_at_index(self, idx: tuple[int, ...]) -> float | None:
             # the data source directly.
             return handle.data()[idx]  # type: ignore [no-any-return]
         return None
+
+    def _auto_scale(self) -> None:
+        if self.lut_model and len(self.handles):
+            policy = self.lut_model.clims
+            handle_clims = [policy.calc_clims(handle.data()) for handle in self.handles]
+            mi, ma = handle_clims[0]
+            for clims in handle_clims[1:]:
+                mi = min(mi, clims[0])
+                ma = max(ma, clims[1])
+
+            for view in self.lut_views:
+                view.set_clims((mi, ma))

src/ndv/models/_lut_model.py

@@ -64,6 +64,13 @@ class ClimsManual(ClimPolicy):
     def get_limits(self, data: npt.NDArray) -> tuple[float, float]:
         return self.min, self.max
 
+    def __eq__(self, other: object) -> bool:
+        return (
+            isinstance(other, ClimsManual)
+            and self.min == other.min
+            and self.max == other.max
+        )
+
 
 class ClimsMinMax(ClimPolicy):
     """Autoscale contrast limits based on the minimum and maximum values in the data."""
@@ -73,6 +80,9 @@ class ClimsMinMax(ClimPolicy):
     def get_limits(self, data: npt.NDArray) -> tuple[float, float]:
         return (np.nanmin(data), np.nanmax(data))
 
+    def __eq__(self, other: object) -> bool:
+        return isinstance(other, ClimsMinMax)
+
 
 class ClimsPercentile(ClimPolicy):
     """Autoscale contrast limits based on percentiles of the data.
@@ -92,6 +102,13 @@ class ClimsPercentile(ClimPolicy):
     def get_limits(self, data: npt.NDArray) -> tuple[float, float]:
         return tuple(np.nanpercentile(data, [self.min_percentile, self.max_percentile]))
 
+    def __eq__(self, other: object) -> bool:
+        return (
+            isinstance(other, ClimsPercentile)
+            and self.min_percentile == other.min_percentile
+            and self.max_percentile == other.max_percentile
+        )
+
 
 class ClimsStdDev(ClimPolicy):
     """Automatically set contrast limits based on standard deviations from the mean.
@@ -114,6 +131,13 @@ def get_limits(self, data: npt.NDArray) -> tuple[float, float]:
         diff = self.n_stdev * np.nanstd(data)
         return center - diff, center + diff
 
+    def __eq__(self, other: object) -> bool:
+        return (
+            isinstance(other, ClimsStdDev)
+            and self.n_stdev == other.n_stdev
+            and self.center == other.center
+        )
+
 
 # we can add this, but it needs to have a proper pydantic serialization method
 # similar to ReducerType

src/ndv/v1/_old_viewer.py

@@ -574,11 +574,11 @@ def _update_canvas_data(self, data: np.ndarray, index: Indices) -> None:
             )
             if datum.ndim == 2:
                 handle = self._canvas.add_image(datum)
-                handle.set_cmap(cm)
+                handle.set_colormap(cm)
                 handles.append(handle)
             elif datum.ndim == 3:
                 handle = self._canvas.add_volume(datum)
-                handle.set_cmap(cm)
+                handle.set_colormap(cm)
                 handles.append(handle)
             if imkey not in self._lut_ctrls:
                 ch_index = index.get(self._channel_axis, 0)

src/ndv/v1/_qt/_lut_control.py

@@ -52,7 +52,7 @@ def __init__(
         self._cmap = CmapCombo()
         self._cmap.currentColormapChanged.connect(self._on_cmap_changed)
         for handle in self._handles:
-            self._cmap.addColormap(handle.cmap())
+            self._cmap.addColormap(handle.colormap())
         for color in cmaplist:
             self._cmap.addColormap(color)
 
@@ -110,7 +110,7 @@ def _on_visible_changed(self, visible: bool) -> None:
 
     def _on_cmap_changed(self, cmap: cmap.Colormap) -> None:
         for handle in self._handles:
-            handle.set_cmap(cmap)
+            handle.set_colormap(cmap)
 
     def update_autoscale(self) -> None:
         if (
@@ -139,5 +139,5 @@ def update_autoscale(self) -> None:
 
     def add_handle(self, handle: ImageHandle) -> None:
         self._handles.append(handle)
-        self._cmap.addColormap(handle.cmap())
+        self._cmap.addColormap(handle.colormap())
         self.update_autoscale()