glue-jupyter icon indicating copy to clipboard operation
glue-jupyter copied to clipboard

POC: Refactor react

Open maartenbreddels opened this issue 2 years ago • 1 comments

This is a proof of concept of applying https://github.com/widgetti/react-ipywidgets to glue-jupyter.

https://github.com/glue-viz/glue-jupyter/pull/273 was the inspiration to develop react-ipywidgets since it is another extreme example of error-prone simple bookkeeping (i.e. syncing state between 2 stateful libraries).

Although this PR is quite large, the second commit, or rather, this diff:

 def create_scale(viewer_state, name):
+    is_log, set_x_log = use_echo_state(viewer_state, f"{name}_log")
     v_min, set_v_min = use_echo_state(viewer_state, f"{name}_min")
     v_max, set_v_max = use_echo_state(viewer_state, f"{name}_max")
-    scale = bq.LinearScale(min=v_min, max=v_max, allow_padding=False)
+    if is_log:
+        scale = bq.LogScale(min=v_min, max=v_max, allow_padding=False)
+    else:
+        scale = bq.LinearScale(min=v_min, max=v_max, allow_padding=False)
     return scale

is the complete replacement of #273, or rather:

-         self.scale_x = bqplot.LinearScale(min=0, max=1, allow_padding=False)
-        self.scale_y = bqplot.LinearScale(min=0, max=1)
+
+        # lazily create widgets
+        self.scale_y_log = None
+        self.scale_y_linear = None
+        self.scale_x_log = None
+        self.scale_x_linear = None
+
+        if self.state.y_log:
+            self.scale_y_log = bqplot.LogScale()
+            self.scale_y = self.scale_y_log
+        else:
+            self.scale_y_linear = bqplot.LinearScale(min=0, max=1)
+            self.scale_y = self.scale_y_linear
+
+        if self.state.x_log:
+            self.scale_x_log = bqplot.LogScale()
+            self.scale_x = self.scale_x_log
+        else:
+            self.scale_x_linear = bqplot.LinearScale(min=0, max=1)
+            self.scale_x = self.scale_x_linear
+
+        def update_scale_type_y(_ignore):
+            if self.state.y_log:
+                if self.scale_y_log is None:
+                    self.scale_y_log = bqplot.LogScale()
+                scale = self.scale_y_log
+            else:
+                if self.scale_y_linear is None:
+                    self.scale_y_linear = bqplot.LinearScale()
+                scale = self.scale_y_linear
+            self.scale_y = scale
+        self.state.add_callback('y_log', update_scale_type_y, priority=-1)
+
+        def update_scale_type_x(_ignore):
+            if self.state.x_log:
+                if self.scale_x_log is None:
+                    self.scale_x_log = bqplot.LogScale()
+                scale = self.scale_x_log
+            else:
+                if self.scale_x_linear is None:
+                    self.scale_x_linear = bqplot.LinearScale()
+                scale = self.scale_x_linear
+            self.scale_y = scale
+        self.state.add_callback('x_log', update_scale_type_x, priority=-1)

Which is significantly less code and therefore less likely to be buggy (I might have missed a _x or mistyped one).

Using react (for ipywidgets) is not without its own problem, mostly a rather steep learning curve.

I do like the replacement of the LayerArtist by simply 1 function:

@react.component
def Histogram(
    scale_x,
    scale_y,
    viewer_state: HistogramViewerState,
    layer_state: HistogramLayerState,
):

I'm not sure it's wise to adopt react-ipywidget in the near term, but I think this PR is useful to have as an example of what this would look like.

maartenbreddels avatar Apr 12 '22 15:04 maartenbreddels

Thanks for the PR! Could you try if adding react_ipywidgets to either install_requires or test in setup.cfg will allow the CI tests to run?

dhomeier avatar Apr 25 '22 17:04 dhomeier