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

POC: Refactor react #308

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

maartenbreddels
Copy link
Collaborator

@maartenbreddels maartenbreddels commented Apr 12, 2022

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

#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.

@dhomeier dhomeier added enhancement New feature or request bqplot-viewers labels Apr 12, 2022
@dhomeier
Copy link
Contributor

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?

Copy link
Member

@astrofrog astrofrog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks interesting, and agree it would simplify a lot of boilerplate code! I can't fully wrap my head around some of the aspects at the moment, in particular the calls to use_echo_state without return values and what side effects that has - could you clarify this a little?

@@ -1,148 +0,0 @@
import numpy as np
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though the new code is much more concise, could you maybe keep the separation between files of the layer artist and viewer and so on? (just as it is a pretty common pattern I have been using for viewers so I find it easier to navigate)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

def calculate_bins(
viewer_state: HistogramViewerState, layer_state: HistogramLayerState
):
use_echo_state(viewer_state, "hist_x_max")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What effect does this line have in practice as no return value is being used?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that is the 'magic' from react, combined with having 2 places to store the state. The idea is that if you use use_echo_state, your 'render function' (i.e. the function wrapped in @react.component) will be re-executed. Typically, you'd use the return value, but they are also directly accessible using e.g. viewer_state.normalize etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bqplot-viewers enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants