-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
[ENH] Learner widgets: Inform about potential problems when overriding preprocessors #5710
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
Changes from all commits
a08cf60
463369a
a0ac61b
8c42dbe
e44754d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -79,6 +79,14 @@ class Error(OWWidget.Error): | |
| class Warning(OWWidget.Warning): | ||
| outdated_learner = Msg("Press Apply to submit changes.") | ||
|
|
||
| class Information(OWWidget.Information): | ||
| ignored_preprocessors = Msg( | ||
| "Ignoring default preprocessing.\n" | ||
| "Default preprocessing, such as scaling, one-hot encoding and " | ||
| "treatment of missing data, has been replaced with user-specified " | ||
| "preprocessors. Problems may occur if these are inadequate " | ||
| "for the given data.") | ||
|
|
||
| class Inputs: | ||
| data = Input("Data", Table) | ||
| preprocessor = Input("Preprocessor", Preprocess) | ||
|
|
@@ -90,6 +98,8 @@ class Outputs: | |
|
|
||
| OUTPUT_MODEL_NAME = Outputs.model.name # Attr for backcompat w/ self.send() code | ||
|
|
||
| _SEND, _SOFT, _UPDATE = range(3) | ||
|
|
||
| def __init__(self, preprocessors=None): | ||
| super().__init__() | ||
| self.__default_learner_name = "" | ||
|
|
@@ -99,6 +109,7 @@ def __init__(self, preprocessors=None): | |
| self.model = None | ||
| self.preprocessors = preprocessors | ||
| self.outdated_settings = False | ||
| self.__apply_level = [] | ||
|
|
||
| self.setup_layout() | ||
| QTimer.singleShot(0, getattr(self, "unconditional_apply", self.apply)) | ||
|
|
@@ -144,7 +155,8 @@ def set_default_learner_name(self, name: str) -> None: | |
| @Inputs.preprocessor | ||
| def set_preprocessor(self, preprocessor): | ||
| self.preprocessors = preprocessor | ||
| self.apply() | ||
| # invalidate learner and model, so handleNewSignals will renew them | ||
| self.learner = self.model = None | ||
|
|
||
| @Inputs.data | ||
| @check_sql_input | ||
|
|
@@ -164,23 +176,50 @@ def set_data(self, data): | |
| "Select one with the Select Columns widget.") | ||
| self.data = None | ||
|
|
||
| self.update_model() | ||
| # invalidate the model so that handleNewSignals will update it | ||
| self.model = None | ||
|
|
||
|
|
||
| def apply(self): | ||
| level, self.__apply_level = max(self.__apply_level, default=self._UPDATE), [] | ||
| """Applies learner and sends new model.""" | ||
| self.update_learner() | ||
| self.update_model() | ||
| if level == self._SEND: | ||
| self._send_learner() | ||
| self._send_model() | ||
| elif level == self._UPDATE: | ||
| self.update_learner() | ||
| self.update_model() | ||
| else: | ||
| self.learner or self.update_learner() | ||
| self.model or self.update_model() | ||
|
|
||
| def apply_as(self, level, unconditional=False): | ||
| self.__apply_level.append(level) | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is horrendous, but I see no way around it (while maintaining compatibility). This class and derived classes used To clean the mess, yet keep the compatilbility, I had a solution in which To make it even more tricky, the function as defined here is called only when auto apply is on. Hence, |
||
| if unconditional: | ||
| self.unconditional_apply() | ||
| else: | ||
| self.apply() | ||
|
|
||
| def update_learner(self): | ||
| self.learner = self.create_learner() | ||
| if self.learner and issubclass(self.LEARNER, Fitter): | ||
| self.learner.use_default_preprocessors = True | ||
| if self.learner is not None: | ||
| self.learner.name = self.effective_learner_name() | ||
| self._send_learner() | ||
|
|
||
| def _send_learner(self): | ||
| self.Outputs.learner.send(self.learner) | ||
| self.outdated_settings = False | ||
| self.Warning.outdated_learner.clear() | ||
|
|
||
| def handleNewSignals(self): | ||
| self.apply_as(self._SOFT, True) | ||
| self.Information.ignored_preprocessors( | ||
| shown=not getattr(self.learner, "use_default_preprocessors", False) | ||
| and getattr(self.LEARNER, "preprocessors", False) | ||
| and self.preprocessors is not None) | ||
|
|
||
| def show_fitting_failed(self, exc): | ||
| """Show error when fitting fails. | ||
| Derived widgets can override this to show more specific messages.""" | ||
|
|
@@ -197,6 +236,9 @@ def update_model(self): | |
| else: | ||
| self.model.name = self.learner_name or self.captionTitle | ||
| self.model.instances = self.data | ||
| self._send_model() | ||
|
|
||
| def _send_model(self): | ||
| self.Outputs.model.send(self.model) | ||
|
|
||
| def check_data(self): | ||
|
|
@@ -223,15 +265,12 @@ def settings_changed(self, *args, **kwargs): | |
| self.Warning.outdated_learner(shown=not self.auto_apply) | ||
| self.apply() | ||
|
|
||
| def _change_name(self, instance, output): | ||
| if instance: | ||
| instance.name = self.effective_learner_name() | ||
| if self.auto_apply: | ||
| output.send(instance) | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was incorrect because it didn't set dirty flag, but was necessary due to broken signal handling. |
||
|
|
||
| def learner_name_changed(self): | ||
| self._change_name(self.learner, self.Outputs.learner) | ||
| self._change_name(self.model, self.Outputs.model) | ||
| if self.model is not None: | ||
| self.model.name = self.effective_learner_name() | ||
| if self.learner is not None: | ||
| self.learner.name = self.effective_learner_name() | ||
| self.apply_as(self._SEND) | ||
|
|
||
| def effective_learner_name(self): | ||
| """Return the effective learner name.""" | ||
|
|
@@ -272,7 +311,6 @@ def add_main_layout(self): | |
| Override this method for laying out any learner-specific parameter controls. | ||
| See setup_layout() method for execution order. | ||
| """ | ||
| pass | ||
|
|
||
| def add_classification_layout(self, box): | ||
| """Creates layout for classification specific options. | ||
|
|
@@ -281,7 +319,6 @@ def add_classification_layout(self, box): | |
| and regression learners require different options. | ||
| See `setup_layout()` method for execution order. | ||
| """ | ||
| pass | ||
|
|
||
| def add_regression_layout(self, box): | ||
| """Creates layout for regression specific options. | ||
|
|
@@ -290,7 +327,6 @@ def add_regression_layout(self, box): | |
| and regression learners require different options. | ||
| See `setup_layout()` method for execution order. | ||
| """ | ||
| pass | ||
|
|
||
| def add_learner_name_widget(self): | ||
| self.name_line_edit = gui.lineEdit( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| from unittest.mock import Mock | ||
| from unittest.mock import Mock, patch | ||
|
|
||
| import scipy.sparse as sp | ||
|
|
||
|
|
@@ -218,3 +218,73 @@ def check_name(name): | |
| check_name("Bar") | ||
| w.set_default_learner_name("") | ||
| check_name("Blarg") | ||
|
|
||
| def test_preprocessor_warning(self): | ||
| class TestLearnerNoPreprocess(Learner): | ||
| name = "Test" | ||
| __returns__ = Mock() | ||
|
|
||
| class TestWidgetNoPreprocess(OWBaseLearner): | ||
| name = "Test" | ||
| LEARNER = TestLearnerNoPreprocess | ||
|
|
||
| class TestLearnerPreprocess(Learner): | ||
| name = "Test" | ||
| preprocessors = [Mock()] | ||
| __returns__ = Mock() | ||
|
|
||
| class TestWidgetPreprocess(OWBaseLearner): | ||
| name = "Test" | ||
| LEARNER = TestLearnerPreprocess | ||
|
|
||
| class TestFitterPreprocess(Fitter): | ||
| name = "Test" | ||
| preprocessors = [Mock()] | ||
| __returns__ = Mock() | ||
|
|
||
| class TestWidgetPreprocessFit(OWBaseLearner): | ||
| name = "Test" | ||
| LEARNER = TestFitterPreprocess | ||
|
|
||
| wno = self.create_widget(TestWidgetNoPreprocess) | ||
| wyes = self.create_widget(TestWidgetPreprocess) | ||
| wfit = self.create_widget(TestWidgetPreprocessFit) | ||
|
|
||
| self.assertFalse(wno.Information.ignored_preprocessors.is_shown()) | ||
| self.assertFalse(wyes.Information.ignored_preprocessors.is_shown()) | ||
| self.assertFalse(wfit.Information.ignored_preprocessors.is_shown()) | ||
|
|
||
| pp = continuize.Continuize() | ||
| self.send_signal(wno.Inputs.preprocessor, pp) | ||
| self.send_signal(wyes.Inputs.preprocessor, pp) | ||
| self.send_signal(wfit.Inputs.preprocessor, pp) | ||
|
|
||
| self.assertFalse(wno.Information.ignored_preprocessors.is_shown()) | ||
| self.assertTrue(wyes.Information.ignored_preprocessors.is_shown()) | ||
| self.assertFalse(wfit.Information.ignored_preprocessors.is_shown()) | ||
|
|
||
| self.send_signal(wno.Inputs.preprocessor, None) | ||
| self.send_signal(wyes.Inputs.preprocessor, None) | ||
| self.send_signal(wfit.Inputs.preprocessor, None) | ||
|
|
||
| self.assertFalse(wno.Information.ignored_preprocessors.is_shown()) | ||
| self.assertFalse(wyes.Information.ignored_preprocessors.is_shown()) | ||
| self.assertFalse(wfit.Information.ignored_preprocessors.is_shown()) | ||
|
|
||
| def test_multiple_sends(self): | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test would fail on master because learner would be sent twice. |
||
| class TestLearner(Learner): | ||
| name = "Test" | ||
| __returns__ = Mock() | ||
|
|
||
| class TestWidget(OWBaseLearner): | ||
| name = "Test" | ||
| LEARNER = TestLearner | ||
|
|
||
| widget = self.create_widget(TestWidget) | ||
| pp = continuize.Continuize() | ||
| with patch.object(widget.Outputs.learner, "send") as model_send, \ | ||
| patch.object(widget.Outputs.model, "send") as learner_send: | ||
| self.send_signals([(widget.Inputs.data, self.iris), | ||
| (widget.Inputs.preprocessor, pp)]) | ||
| learner_send.assert_called_once() | ||
| model_send.assert_called_once() | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should have called
unconditional_apply. Now the base class defineshandleNewSignalsand handles them properly (I suppose).