Merge pull request #4254 from janezd/select-rows-change-vartype

[FIX] Select Rows: Fix crash on changed variable type

Orange/widgets/data/owselectrows.py

@@ -44,26 +44,47 @@ def is_valid_item(self, setting, condition, attrs, metas):
         return varname in attrs or varname in metas
 
     def encode_setting(self, context, setting, value):
-        if setting.name == 'conditions':
-            CONTINUOUS = vartype(ContinuousVariable("x"))
-            for i, (attr, op, values) in enumerate(value):
-                if context.attributes.get(attr) == CONTINUOUS:
-                    if values and isinstance(values[0], str):
-                        values = [QLocale().toDouble(v)[0] for v in values]
-                        value[i] = (attr, op, values)
-        return super().encode_setting(context, setting, value)
+        if setting.name != 'conditions':
+            return super().encode_settings(context, setting, value)
+
+        encoded = []
+        CONTINUOUS = vartype(ContinuousVariable("x"))
+        for attr, op, values in value:
+            vtype = context.attributes.get(attr)
+            if vtype == CONTINUOUS and values and isinstance(values[0], str):
+                values = [QLocale().toDouble(v)[0] for v in values]
+            encoded.append((attr, vtype, op, values))
+        return encoded
 
     def decode_setting(self, setting, value, domain=None):
         value = super().decode_setting(setting, value, domain)
         if setting.name == 'conditions':
-            for i, (attr, op, values) in enumerate(value):
+            # Use this after 2022/2/2:
+            # for i, (attr, _, op, values) in enumerate(value):
+            for i, condition in enumerate(value):
+                attr = condition[0]
+                op, values = condition[-2:]
+
                 var = attr in domain and domain[attr]
                 if var and var.is_continuous and not isinstance(var, TimeVariable):
-                    value[i] = (attr, op,
-                                list([QLocale().toString(float(i), 'f')
-                                      for i in values]))
+                    values = [QLocale().toString(float(i), 'f') for i in values]
+                value[i] = (attr, op, values)
         return value
 
+    def match(self, context, domain, attrs, metas):
+        if (attrs, metas) == (context.attributes, context.metas):
+            return self.PERFECT_MATCH
+
+        conditions = context.values["conditions"]
+        all_vars = attrs
+        all_vars.update(metas)
+        # Use this after 2022/2/2:
+        # if all(all_vars.get(name) == tpe for name, tpe, *_ in conditions):
+        if all(all_vars.get(name) == tpe if len(rest) == 2 else name in all_vars
+               for name, tpe, *rest in conditions):
+            return 0.5
+        return self.NO_MATCH
+
 
 class FilterDiscreteType(enum.Enum):
     Equal = "Equal"
@@ -105,6 +126,8 @@ class Outputs:
     purge_classes = Setting(False, schema_only=True)
     auto_commit = Setting(True)
 
+    settings_version = 2
+
     Operators = {
         ContinuousVariable: [
             (FilterContinuous.Equal, "equals"),
@@ -692,6 +715,14 @@ def send_report(self):
                  ("Non-matching data",
                   nonmatch_inst > 0 and "{} instances".format(nonmatch_inst))))
 
+    # Uncomment this on 2022/2/2
+    #
+    # @classmethod
+    # def migrate_context(cls, context, version):
+    #     if not version or version < 2:
+    #         # Just remove; can't migrate because variables types are unknown
+    #         context.values["conditions"] = []
+
 
 class CheckBoxPopup(QWidget):
     def __init__(self, var, lc, widget_parent=None, widget=None):

Orange/widgets/data/tests/test_owselectrows.py

@@ -1,13 +1,15 @@
 # Test methods with long descriptive names can omit docstrings
 # pylint: disable=missing-docstring
+import time
+
 from AnyQt.QtCore import QLocale, Qt
 from AnyQt.QtTest import QTest
 from AnyQt.QtWidgets import QLineEdit, QComboBox
 
 import numpy as np
 
 from Orange.data import (
-    Table, ContinuousVariable, StringVariable, DiscreteVariable)
+    Table, ContinuousVariable, StringVariable, DiscreteVariable, Domain)
 from Orange.widgets.data.owselectrows import (
     OWSelectRows, FilterDiscreteType, SelectRowsContextHandler)
 from Orange.widgets.tests.base import WidgetTest, datasets
@@ -16,6 +18,7 @@
 from Orange.widgets.tests.utils import simulate, override_locale
 from Orange.widgets.utils.annotated_data import ANNOTATED_DATA_FEATURE_NAME
 from Orange.tests import test_filename
+from orangewidget.settings import VERSION_KEY
 
 CFValues = {
     FilterContinuous.Equal: ["5.4"],
@@ -132,22 +135,22 @@ def test_stores_settings_in_invariant_locale(self):
         context = self.widget.current_context
         self.send_signal(self.widget.Inputs.data, None)
         saved_condition = context.values["conditions"][0]
-        self.assertEqual(saved_condition[2][0], 5.2)
+        self.assertEqual(saved_condition[3][0], 5.2)
 
     @override_locale(QLocale.C)
     def test_restores_continuous_filter_in_c_locale(self):
         iris = Table("iris")[:5]
         # Settings with string value
         self.widget = self.widget_with_context(
-            iris.domain, [["sepal length", 2, ("5.2",)]])
+            iris.domain, [["sepal length", 102, 2, ("5.2",)]])
         self.send_signal(self.widget.Inputs.data, iris)
 
         values = self.widget.conditions[0][2]
         self.assertTrue(values[0].startswith("5.2"))
 
         # Settings with float value
         self.widget = self.widget_with_context(
-            iris.domain, [["sepal length", 2, (5.2,)]])
+            iris.domain, [["sepal length", 102, 2, (5.2,)]])
         self.send_signal(self.widget.Inputs.data, iris)
 
         values = self.widget.conditions[0][2]
@@ -158,20 +161,33 @@ def test_restores_continuous_filter_in_sl_SI_locale(self):
         iris = Table("iris")[:5]
         # Settings with string value
         self.widget = self.widget_with_context(
-            iris.domain, [["sepal length", 2, ("5.2",)]])
+            iris.domain, [["sepal length", 102, 2, ("5.2",)]])
         self.send_signal(self.widget.Inputs.data, iris)
 
         values = self.widget.conditions[0][2]
         self.assertTrue(values[0].startswith("5,2"))
 
         # Settings with float value
         self.widget = self.widget_with_context(
-            iris.domain, [["sepal length", 2, (5.2,)]])
+            iris.domain, [["sepal length", 102, 2, (5.2,)]])
         self.send_signal(self.widget.Inputs.data, iris)
 
         values = self.widget.conditions[0][2]
         self.assertTrue(values[0].startswith("5,2"))
 
+    @override_locale(QLocale.C)
+    def test_partial_matches(self):
+        iris = Table("iris")
+        domain = iris.domain
+        self.widget = self.widget_with_context(
+            domain, [[domain[0].name, 2, ("5.2",)]])
+        iris2 = iris.transform(Domain(domain.attributes[:2], None))
+        self.send_signal(self.widget.Inputs.data, iris2)
+        condition = self.widget.conditions[0]
+        self.assertEqual(condition[0], "sepal length")
+        self.assertEqual(condition[1], 2)
+        self.assertTrue(condition[2][0].startswith("5.2"))
+
     def test_load_settings(self):
         iris = Table("iris")[:5]
         self.send_signal(self.widget.Inputs.data, iris)
@@ -242,10 +258,65 @@ def test_annotated_data(self):
         np.testing.assert_equal(annotations[:50], True)
         np.testing.assert_equal(annotations[50:], False)
 
+    def test_change_var_type(self):
+        iris = Table("iris")
+        domain = iris.domain
+
+        self.send_signal(self.widget.Inputs.data, iris)
+        self.widget.remove_all_button.click()
+        self.enterFilter(domain[0], "is below", "5.2")
+
+        var0vals = list({str(x) for x in iris.X[:, 0]})
+        new_domain = Domain(
+            (DiscreteVariable(domain[0].name, values=var0vals), )
+            + domain.attributes[1:],
+            domain.class_var)
+        new_iris = iris.transform(new_domain)
+        self.send_signal(self.widget.Inputs.data, new_iris)
+
+    # Uncomment this on 2022/2/2
+    #
+    # def test_migration_to_version_1(self):
+    #     iris = Table("iris")
+    #
+    #     ch = SelectRowsContextHandler()
+    #     context = ch.new_context(iris.domain, *ch.encode_domain(iris.domain))
+    #     context.values = dict(conditions=[["petal length", 2, (5.2,)]])
+    #     settings = dict(context_settings=[context])
+    #     widget = self.create_widget(OWSelectRows, settings)
+    #     self.assertEqual(widget.conditions, [])
+
+    @override_locale(QLocale.C)
+    def test_support_old_settings(self):
+        iris = Table("iris")
+        self.widget = self.widget_with_context(
+            iris.domain, [["sepal length", 2, ("5.2",)]])
+        self.send_signal(self.widget.Inputs.data, iris)
+        condition = self.widget.conditions[0]
+        self.assertEqual(condition[0], "sepal length")
+        self.assertEqual(condition[1], 2)
+        self.assertTrue(condition[2][0].startswith("5.2"))
+
+    def test_end_support_for_version_1(self):
+        if time.gmtime() >= (2022, 2, 2):
+            self.fail("""
+Happy 22/2/2!
+
+Now remove support for version==None settings in
+SelectRowsContextHandler.decode_setting and SelectRowsContextHandler.match,
+and uncomment OWSelectRows.migrate.
+
+In tests, uncomment test_migration_to_version_1,
+and remove test_support_old_settings and this test.
+
+Basically, revert this commit.
+""")
+
     def widget_with_context(self, domain, conditions):
         ch = SelectRowsContextHandler()
         context = ch.new_context(domain, *ch.encode_domain(domain))
-        context.values = dict(conditions=conditions)
+        context.values = {"conditions": conditions,
+                          VERSION_KEY: OWSelectRows.settings_version}
         settings = dict(context_settings=[context])
 
         return self.create_widget(OWSelectRows, settings)