From c0599cdf63582d9b098d795671370532d8ec8daf Mon Sep 17 00:00:00 2001 From: Ales Erjavec Date: Thu, 26 Jan 2017 16:14:58 +0100 Subject: [PATCH 1/5] owimpute: Fix reset of individual imputers to default Fixes gh-1920 --- Orange/widgets/data/owimpute.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Orange/widgets/data/owimpute.py b/Orange/widgets/data/owimpute.py index b10d70506aa..8fa5fa5ea31 100644 --- a/Orange/widgets/data/owimpute.py +++ b/Orange/widgets/data/owimpute.py @@ -352,7 +352,7 @@ def set_method_for_current_selection(self, method_index): def set_method_for_indexes(self, indexes, method_index): if method_index == self.DEFAULT: for index in indexes: - self.variable_methods.pop(index, None) + self.variable_methods.pop(index.row(), None) else: method = self.METHODS[method_index].copy() for index in indexes: @@ -386,7 +386,7 @@ def _on_value_changed(self): self.set_method_for_current_selection(index) def reset_variable_methods(self): - indexes = map(self.varmodel.index, range(len(self.varmodel))) + indexes = list(map(self.varmodel.index, range(len(self.varmodel)))) self.set_method_for_indexes(indexes, self.DEFAULT) self.variable_button_group.button(self.DEFAULT).setChecked(True) From 2f8bbc3d9a17e1dcdeea48e396d8f8682bbc556e Mon Sep 17 00:00:00 2001 From: Ales Erjavec Date: Thu, 26 Jan 2017 18:21:39 +0100 Subject: [PATCH 2/5] owimpute: Add tests for method selection --- Orange/widgets/data/tests/test_owimpute.py | 53 ++++++++++++++++++++-- 1 file changed, 50 insertions(+), 3 deletions(-) diff --git a/Orange/widgets/data/tests/test_owimpute.py b/Orange/widgets/data/tests/test_owimpute.py index 53bd440cc4a..9613f90372d 100644 --- a/Orange/widgets/data/tests/test_owimpute.py +++ b/Orange/widgets/data/tests/test_owimpute.py @@ -3,13 +3,14 @@ import numpy as np from Orange.data import Table -from Orange.widgets.data.owimpute import OWImpute +from Orange.preprocess import impute +from Orange.widgets.data.owimpute import OWImpute, AsDefault from Orange.widgets.tests.base import WidgetTest - +from Orange.widgets.utils.itemmodels import select_row class TestOWImpute(WidgetTest): def setUp(self): - self.widget = self.create_widget(OWImpute) + self.widget = self.create_widget(OWImpute) # type: OWImpute def test_empty_data(self): """No crash on empty data""" @@ -43,3 +44,49 @@ def test_no_features(self): self.send_signal("Learner", lambda *_: (lambda *_: 1/0)) # Model fails widget.unconditional_commit() self.assertTrue(widget.Error.imputation_failed.is_shown()) + + def test_select_method(self): + data = Table("iris")[::5] + + self.send_signal("Data", data) + method_types = [type(m) for m in OWImpute.METHODS] + + widget = self.widget + model = widget.varmodel + view = widget.varview + defbg = widget.default_button_group + varbg = widget.variable_button_group + self.assertSequenceEqual(list(model), data.domain.variables) + asdefid = method_types.index(AsDefault) + leaveid = method_types.index(impute.DoNotImpute) + avgid = method_types.index(impute.Average) + defbg.button(avgid).click() + self.assertEqual(widget.default_method_index, avgid) + + self.assertTrue( + all(isinstance(m, AsDefault) and isinstance(m.method, impute.Average) + for m in map(widget.get_method_for_column, + range(len(data.domain.variables))))) + + # change method for first variable + select_row(view, 0) + varbg.button(avgid).click() + met = widget.get_method_for_column(0) + self.assertIsInstance(met, impute.Average) + + # select a second var + selmodel = view.selectionModel() + selmodel.select(model.index(2), selmodel.Select) + # the current checked button must unset + self.assertEqual(varbg.checkedId(), -1) + + varbg.button(leaveid).click() + self.assertIsInstance(widget.get_method_for_column(0), + impute.DoNotImpute) + self.assertIsInstance(widget.get_method_for_column(2), + impute.DoNotImpute) + + # reset both back to default + varbg.button(asdefid).click() + self.assertIsInstance(widget.get_method_for_column(0), AsDefault) + self.assertIsInstance(widget.get_method_for_column(2), AsDefault) From 602783e2064cd554b04edfcf773d7eb10e54ac53 Mon Sep 17 00:00:00 2001 From: Ales Erjavec Date: Thu, 26 Jan 2017 19:29:29 +0100 Subject: [PATCH 3/5] owimpute: Use/modify only instance local imputer methods --- Orange/widgets/data/owimpute.py | 33 +++++++++++++--------- Orange/widgets/data/tests/test_owimpute.py | 5 ++-- 2 files changed, 22 insertions(+), 16 deletions(-) diff --git a/Orange/widgets/data/owimpute.py b/Orange/widgets/data/owimpute.py index 8fa5fa5ea31..8e03864661c 100644 --- a/Orange/widgets/data/owimpute.py +++ b/Orange/widgets/data/owimpute.py @@ -1,4 +1,6 @@ import sys +import copy + import numpy as np from AnyQt.QtWidgets import ( @@ -82,6 +84,9 @@ class Error(OWWidget.Error): def __init__(self): super().__init__() + # copy METHODS (some are modified by the widget) + self.methods = copy.deepcopy(OWImpute.METHODS) + main_layout = QVBoxLayout() main_layout.setContentsMargins(10, 10, 10, 10) self.controlArea.layout().addLayout(main_layout) @@ -92,7 +97,7 @@ def __init__(self): button_group = QButtonGroup() button_group.buttonClicked[int].connect(self.set_default_method) - for i, method in enumerate(self.METHODS): + for i, method in enumerate(self.methods): if not method.columns_only: button = QRadioButton(method.name) button.setChecked(i == self.default_method_index) @@ -125,7 +130,7 @@ def __init__(self): horizontal_layout.addLayout(method_layout) button_group = QButtonGroup() - for i, method in enumerate(self.METHODS): + for i, method in enumerate(self.methods): button = QRadioButton(text=method.name) button_group.addButton(button, i) method_layout.addWidget(button) @@ -168,7 +173,7 @@ def __init__(self): self.data = None self.modified = False - self.default_method = self.METHODS[self.default_method_index] + self.default_method = self.methods[self.default_method_index] self.update_varview() @property @@ -180,14 +185,14 @@ def default_method_index(self, index): if self._default_method_index != index: self._default_method_index = index self.default_button_group.button(index).setChecked(True) - self.default_method = self.METHODS[self.default_method_index] - self.METHODS[self.DEFAULT].method = self.default_method + self.default_method = self.methods[self.default_method_index] + self.methods[self.DEFAULT].method = self.default_method # update variable view for index in map(self.varmodel.index, range(len(self.varmodel))): - self.varmodel.setData(index, - self.variable_methods.get(index.row(), self.METHODS[self.DEFAULT]), - Qt.UserRole) + method = self.variable_methods.get( + index.row(), self.methods[self.DEFAULT]) + self.varmodel.setData(index, method, Qt.UserRole) self._invalidate() def set_default_method(self, index): @@ -212,7 +217,7 @@ def set_data(self, data): def set_learner(self, learner): self.learner = learner or self.DEFAULT_LEARNER - imputer = self.METHODS[self.MODEL_BASED_IMPUTER] + imputer = self.methods[self.MODEL_BASED_IMPUTER] imputer.learner = self.learner button = self.default_button_group.button(self.MODEL_BASED_IMPUTER) @@ -233,7 +238,7 @@ def get_method_for_column(self, column_index): column_index = column_index.row() return self.variable_methods.get(column_index, - self.METHODS[self.DEFAULT]) + self.methods[self.DEFAULT]) def _invalidate(self): self.modified = True @@ -317,7 +322,7 @@ def _on_var_selection_changed(self): if len(methods) == 1: method = methods.pop() - for i, m in enumerate(self.METHODS): + for i, m in enumerate(self.methods): if method == m.name: self.variable_button_group.button(i).setChecked(True) elif self.variable_button_group.checkedButton() is not None: @@ -325,7 +330,7 @@ def _on_var_selection_changed(self): self.variable_button_group.checkedButton().setChecked(False) self.variable_button_group.setExclusive(True) - for method, button in zip(self.METHODS, + for method, button in zip(self.methods, self.variable_button_group.buttons()): enabled = all(method.supports_variable(var) for var in selected_vars) @@ -354,7 +359,7 @@ def set_method_for_indexes(self, indexes, method_index): for index in indexes: self.variable_methods.pop(index.row(), None) else: - method = self.METHODS[method_index].copy() + method = self.methods[method_index].copy() for index in indexes: self.variable_methods[index.row()] = method @@ -380,7 +385,7 @@ def _on_value_changed(self): value = self.value_double.value() self.default_value = value - self.METHODS[self.AS_INPUT].default = value + self.methods[self.AS_INPUT].default = value index = self.variable_button_group.checkedId() if index == self.AS_INPUT: self.set_method_for_current_selection(index) diff --git a/Orange/widgets/data/tests/test_owimpute.py b/Orange/widgets/data/tests/test_owimpute.py index 9613f90372d..a1cafe840d8 100644 --- a/Orange/widgets/data/tests/test_owimpute.py +++ b/Orange/widgets/data/tests/test_owimpute.py @@ -8,6 +8,7 @@ from Orange.widgets.tests.base import WidgetTest from Orange.widgets.utils.itemmodels import select_row + class TestOWImpute(WidgetTest): def setUp(self): self.widget = self.create_widget(OWImpute) # type: OWImpute @@ -17,7 +18,7 @@ def test_empty_data(self): data = Table("iris") widget = self.widget widget.default_method_index = widget.MODEL_BASED_IMPUTER - widget.default_method = widget.METHODS[widget.default_method_index] + widget.default_method = widget.methods[widget.default_method_index] self.send_signal("Data", data) widget.unconditional_commit() @@ -33,7 +34,7 @@ def test_empty_data(self): def test_no_features(self): widget = self.widget widget.default_method_index = widget.MODEL_BASED_IMPUTER - widget.default_method = widget.METHODS[widget.default_method_index] + widget.default_method = widget.methods[widget.default_method_index] self.send_signal("Data", Table("iris")) From fdd66faa154bf6797e59694b907a71700c74762c Mon Sep 17 00:00:00 2001 From: Ales Erjavec Date: Mon, 30 Jan 2017 16:54:45 +0100 Subject: [PATCH 4/5] owimpute: Change QStackedLayout -> QStackedWidget Fix use of the value_stack.setEnabled(...) which was used like it was a QStackedWidget. --- Orange/widgets/data/owimpute.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Orange/widgets/data/owimpute.py b/Orange/widgets/data/owimpute.py index 8e03864661c..800f04adc48 100644 --- a/Orange/widgets/data/owimpute.py +++ b/Orange/widgets/data/owimpute.py @@ -4,8 +4,8 @@ import numpy as np from AnyQt.QtWidgets import ( - QWidget, QGroupBox, QRadioButton, QPushButton, QHBoxLayout, - QVBoxLayout, QStackedLayout, QComboBox, QLineEdit, + QGroupBox, QRadioButton, QPushButton, QHBoxLayout, + QVBoxLayout, QStackedWidget, QComboBox, QButtonGroup, QStyledItemDelegate, QListView, QDoubleSpinBox ) from AnyQt.QtCore import Qt @@ -146,10 +146,10 @@ def __init__(self): minimum=-1000., maximum=1000., singleStep=.1, decimals=3, value=self.default_value ) - self.value_stack = value_stack = QStackedLayout() + self.value_stack = value_stack = QStackedWidget() value_stack.addWidget(self.value_combo) value_stack.addWidget(self.value_double) - method_layout.addLayout(value_stack) + method_layout.addWidget(value_stack) button_group.buttonClicked[int].connect( self.set_method_for_current_selection From 9f64238ea8433a039853579efb2a4d3d6af6fbaf Mon Sep 17 00:00:00 2001 From: Ales Erjavec Date: Mon, 30 Jan 2017 20:03:12 +0100 Subject: [PATCH 5/5] owimpute: Fix per-variable value editing Did not work properly since gh-1094 Fixes gh-1965 --- Orange/widgets/data/owimpute.py | 87 ++++++++++++++-------- Orange/widgets/data/tests/test_owimpute.py | 73 +++++++++++++++++- 2 files changed, 130 insertions(+), 30 deletions(-) diff --git a/Orange/widgets/data/owimpute.py b/Orange/widgets/data/owimpute.py index 800f04adc48..1a2a8815148 100644 --- a/Orange/widgets/data/owimpute.py +++ b/Orange/widgets/data/owimpute.py @@ -77,7 +77,6 @@ class Error(OWWidget.Error): _default_method_index = settings.Setting(DO_NOT_IMPUTE) variable_methods = settings.ContextSetting({}) autocommit = settings.Setting(False) - default_value = settings.Setting(0.) want_main_area = False resizing_enabled = False @@ -140,11 +139,9 @@ def __init__(self): sizeAdjustPolicy=QComboBox.AdjustToMinimumContentsLength, activated=self._on_value_selected ) - self.value_combo.currentIndexChanged.connect(self._on_value_changed) self.value_double = QDoubleSpinBox( editingFinished=self._on_value_selected, minimum=-1000., maximum=1000., singleStep=.1, decimals=3, - value=self.default_value ) self.value_stack = value_stack = QStackedWidget() value_stack.addWidget(self.value_combo) @@ -172,9 +169,9 @@ def __init__(self): box.layout().insertWidget(0, self.report_button) self.data = None + self.learner = None self.modified = False self.default_method = self.methods[self.default_method_index] - self.update_varview() @property def default_method_index(self): @@ -229,6 +226,7 @@ def set_learner(self, learner): if learner is not None: self.default_method_index = self.MODEL_BASED_IMPUTER + self.update_varview() self.commit() def get_method_for_column(self, column_index): @@ -272,7 +270,7 @@ def commit(self): drop_mask |= method(self.data, var) else: var = method(self.data, var) - except: + except Exception: # pylint: disable=broad-except self.Error.imputation_failed(var.name) attributes = class_vars = None break @@ -315,20 +313,44 @@ def send_report(self): def _on_var_selection_changed(self): indexes = self.selection.selectedIndexes() - methods = set(self.get_method_for_column(i.row()).name for i in indexes) + methods = [self.get_method_for_column(i.row()) for i in indexes] + + def method_key(method): + """ + Decompose method into its type and parameters. + """ + # The return value should be hashable and __eq__ comparable + if isinstance(method, AsDefault): + return AsDefault, (method.method,) + elif isinstance(method, impute.Model): + return impute.Model, (method.learner,) + elif isinstance(method, impute.Default): + return impute.Default, (method.default,) + else: + return type(method), None + methods = set(method_key(m) for m in methods) selected_vars = [self.varmodel[index.row()] for index in indexes] has_discrete = any(var.is_discrete for var in selected_vars) + fixed_value = None + value_stack_enabled = False + current_value_widget = None if len(methods) == 1: - method = methods.pop() + method_type, parameters = methods.pop() for i, m in enumerate(self.methods): - if method == m.name: + if method_type == type(m): self.variable_button_group.button(i).setChecked(True) + + if method_type is impute.Default: + (fixed_value,) = parameters + elif self.variable_button_group.checkedButton() is not None: + # Uncheck the current button self.variable_button_group.setExclusive(False) self.variable_button_group.checkedButton().setChecked(False) self.variable_button_group.setExclusive(True) + assert self.variable_button_group.checkedButton() is None for method, button in zip(self.methods, self.variable_button_group.buttons()): @@ -337,18 +359,28 @@ def _on_var_selection_changed(self): button.setEnabled(enabled) if not has_discrete: - self.value_stack.setEnabled(True) - self.value_stack.setCurrentWidget(self.value_double) - self._on_value_changed() + value_stack_enabled = True + current_value_widget = self.value_double elif len(selected_vars) == 1: - self.value_stack.setEnabled(True) - self.value_stack.setCurrentWidget(self.value_combo) + value_stack_enabled = True + current_value_widget = self.value_combo self.value_combo.clear() self.value_combo.addItems(selected_vars[0].values) - self._on_value_changed() else: + value_stack_enabled = False + current_value_widget = None self.variable_button_group.button(self.AS_INPUT).setEnabled(False) - self.value_stack.setEnabled(False) + + self.value_stack.setEnabled(value_stack_enabled) + if current_value_widget is not None: + self.value_stack.setCurrentWidget(current_value_widget) + if fixed_value is not None: + if current_value_widget is self.value_combo: + self.value_combo.setCurrentIndex(fixed_value) + elif current_value_widget is self.value_double: + self.value_double.setValue(fixed_value) + else: + assert False def set_method_for_current_selection(self, method_index): indexes = self.selection.selectedIndexes() @@ -358,6 +390,15 @@ def set_method_for_indexes(self, indexes, method_index): if method_index == self.DEFAULT: for index in indexes: self.variable_methods.pop(index.row(), None) + elif method_index == OWImpute.AS_INPUT: + current = self.value_stack.currentWidget() + if current is self.value_combo: + value = self.value_combo.currentIndex() + else: + value = self.value_double.value() + for index in indexes: + method = impute.Default(default=value) + self.variable_methods[index.row()] = method else: method = self.methods[method_index].copy() for index in indexes: @@ -374,21 +415,9 @@ def update_varview(self, indexes=None): self.varmodel.setData(index, self.get_method_for_column(index.row()), Qt.UserRole) def _on_value_selected(self): + # The fixed 'Value' in the widget has been changed by the user. self.variable_button_group.button(self.AS_INPUT).setChecked(True) - self._on_value_changed() - - def _on_value_changed(self): - widget = self.value_stack.currentWidget() - if widget is self.value_combo: - value = self.value_combo.currentText() - else: - value = self.value_double.value() - self.default_value = value - - self.methods[self.AS_INPUT].default = value - index = self.variable_button_group.checkedId() - if index == self.AS_INPUT: - self.set_method_for_current_selection(index) + self.set_method_for_current_selection(self.AS_INPUT) def reset_variable_methods(self): indexes = list(map(self.varmodel.index, range(len(self.varmodel)))) diff --git a/Orange/widgets/data/tests/test_owimpute.py b/Orange/widgets/data/tests/test_owimpute.py index a1cafe840d8..1b86fbb332c 100644 --- a/Orange/widgets/data/tests/test_owimpute.py +++ b/Orange/widgets/data/tests/test_owimpute.py @@ -2,10 +2,14 @@ # pylint: disable=missing-docstring import numpy as np +from AnyQt.QtCore import Qt, QItemSelection +from AnyQt.QtTest import QTest + from Orange.data import Table from Orange.preprocess import impute from Orange.widgets.data.owimpute import OWImpute, AsDefault from Orange.widgets.tests.base import WidgetTest +from Orange.widgets.tests.utils import simulate from Orange.widgets.utils.itemmodels import select_row @@ -86,8 +90,75 @@ def test_select_method(self): impute.DoNotImpute) self.assertIsInstance(widget.get_method_for_column(2), impute.DoNotImpute) - # reset both back to default varbg.button(asdefid).click() self.assertIsInstance(widget.get_method_for_column(0), AsDefault) self.assertIsInstance(widget.get_method_for_column(2), AsDefault) + + def test_value_edit(self): + data = Table("heart_disease")[::10] + self.send_signal("Data", data) + widget = self.widget + model = widget.varmodel + view = widget.varview + selmodel = view.selectionModel() + varbg = widget.variable_button_group + method_types = [type(m) for m in OWImpute.METHODS] + asdefid = method_types.index(AsDefault) + valueid = method_types.index(impute.Default) + + def selectvars(varlist, command=selmodel.ClearAndSelect): + indices = [data.domain.index(var) for var in varlist] + itemsel = QItemSelection() + for ind in indices: + midx = model.index(ind) + itemsel.select(midx, midx) + selmodel.select(itemsel, command) + + def effective_method(var): + return widget.get_method_for_column(data.domain.index(var)) + + # select 'chest pain' + selectvars(['chest pain']) + self.assertTrue(widget.value_combo.isVisibleTo(widget) and + widget.value_combo.isEnabledTo(widget)) + self.assertEqual(varbg.checkedId(), asdefid) + + simulate.combobox_activate_item( + widget.value_combo, data.domain["chest pain"].values[1]) + # The 'Value' (impute.Default) should have been selected automatically + self.assertEqual(varbg.checkedId(), valueid) + imputer = effective_method('chest pain') + self.assertIsInstance(imputer, impute.Default) + self.assertEqual(imputer.default, 1) + + # select continuous 'rest SBP' and 'cholesterol' variables + selectvars(["rest SBP", "cholesterol"]) + self.assertTrue(widget.value_double.isVisibleTo(widget) and + widget.value_double.isEnabledTo(widget)) + self.assertEqual(varbg.checkedId(), asdefid) + widget.value_double.setValue(-1.0) + QTest.keyClick(self.widget.value_double, Qt.Key_Enter) + # The 'Value' (impute.Default) should have been selected automatically + self.assertEqual(varbg.checkedId(), valueid) + imputer = effective_method("rest SBP") + self.assertIsInstance(imputer, impute.Default) + self.assertEqual(imputer.default, -1.0) + imputer = effective_method("cholesterol") + self.assertIsInstance(imputer, impute.Default) + self.assertEqual(imputer.default, -1.0) + + # Add 'chest pain' to the selection and ensure the value stack is + # disabled + selectvars(["chest pain"], selmodel.Select) + self.assertEqual(varbg.checkedId(), -1) + self.assertFalse(widget.value_combo.isEnabledTo(widget) and + widget.value_double.isEnabledTo(widget)) + + # select 'chest pain' only and check that the selected value is + # restored in the value combo + selectvars(["chest pain"]) + self.assertTrue(widget.value_combo.isVisibleTo(widget) and + widget.value_combo.isEnabledTo(widget)) + self.assertEqual(varbg.checkedId(), valueid) + self.assertEqual(widget.value_combo.currentIndex(), 1)