reworked editor internals to avoid calling update methods several times per change (issue #93)

Author: gdementen

larray_editor/arraywidget.py

@@ -524,7 +524,6 @@ def __init__(self, parent, data_scrollbar):
 gradient_map = dict(available_gradients)
 
 
-
 class ArrayEditorWidget(QWidget):
     def __init__(self, parent, data=None, readonly=False, bg_value=None, bg_gradient='blue-red',
                  minvalue=None, maxvalue=None):
@@ -756,19 +755,21 @@ def dropEvent(self, event):
 
     def set_data(self, data=None, bg_value=None):
         # update adapter
-        self.data_adapter.set_data(data, bg_value=bg_value)
-        la_data = self.data_adapter.get_data()
-        axes = la_data.axes
+        if data is None:
+            data = la.LArray([])
+        else:
+            data = la.aslarray(data)
+        axes = data.axes
         display_names = axes.display_names
 
-        # update data format and bgcolor
-        self._update_digits_scientific(la_data)
+        # update data format
+        self._update_digits_scientific(data)
 
         # update filters
         filters_layout = self.filters_layout
         clear_layout(filters_layout)
         # data.size > 0 to avoid arrays with length 0 axes and len(axes) > 0 to avoid scalars (scalar.size == 1)
-        if la_data.size > 0 and len(axes) > 0:
+        if data.size > 0 and len(axes) > 0:
             filters_layout.addWidget(QLabel(_("Filters")))
             for axis, display_name in zip(axes, display_names):
                 filters_layout.addWidget(QLabel(display_name))
@@ -779,87 +780,96 @@ def set_data(self, data=None, bg_value=None):
                 else:
                     filters_layout.addWidget(QLabel("too big to be filtered"))
             filters_layout.addStretch()
-        self.data_adapter.update_filtered_data({})
+
+        self.gradient_chooser.setEnabled(self.model_data.bgcolor_possible)
+
+        self.data_adapter.set_data(data, bg_value=bg_value)
 
         # reset default size
         self.view_axes.set_default_size()
         self.view_ylabels.set_default_size()
         self.view_xlabels.set_default_size()
         self.view_data.set_default_size()
 
-    def _update_digits_scientific(self, data):
+    # called by set_data and ArrayEditorWidget.accept_changes (this should not be the case IMO)
+    # two cases:
+    # * set_data should update both scientific and ndigits
+    # * toggling scientific checkbox should update only ndigits
+    def _update_digits_scientific(self, data, scientific=None):
         """
         data : LArray
         """
-        # TODO: Adapter must provide a method to return a data sample as a Numpy array
-        assert isinstance(data, la.LArray)
-        data = data.data
-        size, dtype = data.size, data.dtype
-        # this will yield a data sample of max 199
-        step = (size // 100) if size > 100 else 1
-        data_sample = data.flat[::step]
-
-        # TODO: refactor so that the expensive format_helper is not called
-        # twice (or the values are cached)
-        use_scientific = self.choose_scientific(data_sample)
+        dtype = data.dtype
+        if dtype.type in (np.str, np.str_, np.bool_, np.bool, np.object_):
+            scientific = False
+            ndecimals = 0
+        else:
+            # XXX: move this to the adapter (return a data sample as a Numpy array)
+            data = self._get_sample(data)
+
+            # max_digits = self.get_max_digits()
+            # default width can fit 8 chars
+            # FIXME: use max_digits?
+            avail_digits = 8
+            frac_zeros, int_digits, has_negative = self.format_helper(data)
+
+            # choose whether or not to use scientific notation
+            # ================================================
+            if scientific is None:
+                # use scientific format if there are more integer digits than we can display or if we can display more
+                # information that way (scientific format "uses" 4 digits, so we have a net win if we have >= 4 zeros --
+                # *including the integer one*)
+                # TODO: only do so if we would actually display more information
+                # 0.00001 can be displayed with 8 chars
+                # 1e-05
+                # would
+                scientific = int_digits > avail_digits or frac_zeros >= 4
+
+            # determine best number of decimals to display
+            # ============================================
+            # TODO: ndecimals vs self.digits => rename self.digits to either frac_digits or ndecimals
+            data_frac_digits = self._data_digits(data)
+            if scientific:
+                int_digits = 2 if has_negative else 1
+                exp_digits = 4
+            else:
+                exp_digits = 0
+            # - 1 for the dot
+            ndecimals = avail_digits - 1 - int_digits - exp_digits
 
-        # XXX: self.ndecimals vs self.digits
-        self.digits = self.choose_ndecimals(data_sample, use_scientific)
-        self.use_scientific = use_scientific
-        self.model_data.set_format(self.cell_format)
+            if ndecimals < 0:
+                ndecimals = 0
 
-        self.digits_spinbox.setValue(self.digits)
-        self.digits_spinbox.setEnabled(is_number(dtype))
+            if data_frac_digits < ndecimals:
+                ndecimals = data_frac_digits
 
-        self.scientific_checkbox.setChecked(use_scientific)
-        self.scientific_checkbox.setEnabled(is_number(dtype))
+        self.digits = ndecimals
 
-        self.gradient_chooser.setEnabled(self.model_data.bgcolor_possible)
+        self.use_scientific = scientific
 
-    def choose_scientific(self, data):
-        # max_digits = self.get_max_digits()
-        # default width can fit 8 chars
-        # FIXME: use max_digits?
-        avail_digits = 8
-        if data.dtype.type in (np.str, np.str_, np.bool_, np.bool, np.object_):
-            return False
-
-        frac_zeros, int_digits, _ = self.format_helper(data)
-
-        # if there are more integer digits than we can display or we can
-        # display more information by using scientific format, do so
-        # (scientific format "uses" 4 digits, so we win if have >= 4 zeros
-        #  -- *including the integer one*)
-        # TODO: only do so if we would actually display more information
-        # 0.00001 can be displayed with 8 chars
-        # 1e-05
-        # would
-        return int_digits > avail_digits or frac_zeros >= 4
-
-    def choose_ndecimals(self, data, scientific):
-        if data.dtype.type in (np.str, np.str_, np.bool_, np.bool, np.object_):
-            return 0
+        self.digits_spinbox.blockSignals(True)
+        self.digits_spinbox.setValue(ndecimals)
+        self.digits_spinbox.setEnabled(is_number(dtype))
+        self.digits_spinbox.blockSignals(False)
 
-        # max_digits = self.get_max_digits()
-        # default width can fit 8 chars
-        # FIXME: use max_digits?
-        avail_digits = 8
-        data_frac_digits = self._data_digits(data)
-        _, int_digits, negative = self.format_helper(data)
-        if scientific:
-            int_digits = 2 if negative else 1
-            exp_digits = 4
-        else:
-            exp_digits = 0
-        # - 1 for the dot
-        ndecimals = avail_digits - 1 - int_digits - exp_digits
+        self.scientific_checkbox.blockSignals(True)
+        self.scientific_checkbox.setChecked(scientific)
+        self.scientific_checkbox.setEnabled(is_number(dtype))
+        self.scientific_checkbox.blockSignals(False)
 
-        if ndecimals < 0:
-            ndecimals = 0
+        # setting the format explicitly instead of relying on digits_spinbox.digits_changed to set it because
+        # digits_changed is only triggered when digits actually changed, not when passing from scientific -> non
+        # scientific or number -> object
+        self.model_data.set_format(self.cell_format)
 
-        if data_frac_digits < ndecimals:
-            ndecimals = data_frac_digits
-        return ndecimals
+    def _get_sample(self, data):
+        assert isinstance(data, la.LArray)
+        data = data.data
+        size = data.size
+        # this will yield a data sample of max 199
+        step = (size // 100) if size > 100 else 1
+        sample = data.flat[::step]
+        return sample[np.isfinite(sample)]
 
     def format_helper(self, data):
         if not data.size:
@@ -963,10 +973,7 @@ def cell_format(self):
             return '%%.%d%s' % (self.digits, format_letter)
 
     def scientific_changed(self, value):
-        self.use_scientific = value
-        self.digits = self.choose_ndecimals(self.data_adapter.get_data(), value)
-        self.digits_spinbox.setValue(self.digits)
-        self.model_data.set_format(self.cell_format)
+        self._update_digits_scientific(self.data_adapter.get_data(), value)
 
     def digits_changed(self, value):
         self.digits = value

larray_editor/editor.py

@@ -108,9 +108,7 @@ def setup_and_check(self, data, title='', readonly=False, minvalue=None, maxvalu
         widget.setLayout(layout)
 
         self._listwidget = QListWidget(self)
-        self._listwidget.currentItemChanged.connect(self.on_item_changed)
-        # this is a workaround for the fact that no currentItemChanged signal is emitted when no item was selected
-        # before
+        # this is a bit more reliable than currentItemChanged which is not emitted when no item was selected before
         self._listwidget.itemSelectionChanged.connect(self.on_selection_changed)
         self._listwidget.setMinimumWidth(45)
 
@@ -346,16 +344,14 @@ def select_list_item(self, to_display):
             # we need to update the array widget explicitly
             self.set_current_array(self.data[to_display], to_display)
         else:
-            # for some reason, on_item_changed is not triggered when no item was selected
-            if not prev_selected:
-                self.set_current_array(self.data[to_display], to_display)
             self._listwidget.setCurrentItem(changed_items[0])
 
     def update_mapping(self, value):
         # XXX: use ordered set so that the order is non-random if the underlying container is ordered?
         keys_before = set(self.data.keys())
         keys_after = set(value.keys())
-        # contains both new and updated keys (but not deleted keys)
+        # Contains both new and keys for which the object id changed (but not deleted keys nor inplace modified keys).
+        # Inplace modified arrays should be already handled in ipython_cell_executed by the setitem_pattern.
         changed_keys = [k for k in keys_after if value[k] is not self.data.get(k)]
 
         # when a key is re-assigned, it can switch from being displayable to non-displayable or vice versa
@@ -438,6 +434,8 @@ def ipython_cell_executed(self):
             # otherwise it should have failed at this point, but let us be sure
             if varname in clean_ns:
                 if self._display_in_grid(varname, clean_ns[varname]):
+                    # XXX: this completely refreshes the array, including detecting scientific & ndigits, which might
+                    # not be what we want in this case
                     self.select_list_item(varname)
         else:
             # not setitem => assume expr or normal assignment
@@ -448,6 +446,7 @@ def ipython_cell_executed(self):
                     self.select_list_item(last_input)
             else:
                 # any statement can contain a call to a function which updates globals
+                # this will select (or refresh) the "first" changed array
                 self.update_mapping(clean_ns)
 
                 # if the statement produced any output (probably because it is a simple expression), display it.
@@ -469,11 +468,7 @@ def on_selection_changed(self, *args, **kwargs):
             assert len(selected) == 1
             selected_item = selected[0]
             assert isinstance(selected_item, QListWidgetItem)
-            self.on_item_changed(selected_item, None)
-
-    def on_item_changed(self, curr, prev):
-        if curr is not None:
-            name = str(curr.text())
+            name = str(selected_item.text())
             array = self.data[name]
             self.set_current_array(array, name)
             expr = self.expressions.get(name, name)