OWImpute: fix model based imputer. Fixes #1082. by s-alexey · Pull Request #1094 · biolab/orange3

s-alexey · 2016-03-10T00:37:40Z

Switch to Model-based imputer in case learner is provided.

Added learner adequacy checker.

codecov-io · 2016-03-10T00:43:23Z

Current coverage is 86.15%

Merging #1094 into master will decrease coverage by -1.05%

@@             master      #1094   diff @@
==========================================
  Files            76         77     +1   
  Lines          7330       7503   +173   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           6392       6464    +72   
- Misses          938       1039   +101   
  Partials          0          0

2 files (not in diff) in Orange/data were modified. more
- Misses +2
- Hits -2
2 files (not in diff) in ...range/classification were modified. more
- Misses +1
File ...lassification/mlp.py (not in diff) was created. more

Powered by Codecov. Last updated by 5da5f03...1fba4a6

astaric · 2016-04-15T11:46:41Z

I like the fix as it is simple and straightforward. The problem is that the change on the Model imputer now allows the user to construct a model imputer for a variable of the wrong type, which would not impute anything. This is not desired.

I would rather move the check to the widget, and use some kind of default_learner for the variables with incompatible type (rather than not impute them).

If Model based imputer option was renamed to Model based imputer (), this would also signal to the user which learner will be used for imputation. When a given learner only supports one kind of variables, text could dynamically change to "Model base imputer (continuous: , discrete: " or something like this.

s-alexey · 2016-04-16T20:06:18Z

@astaric I have changed my PR.
It's not quite what you suggested but hope you will like it.

astaric · 2016-04-18T08:09:35Z

Orange/preprocess/impute.py

+        return Model(self.learner)
+
+    @property
+    def support_discrete(self):


What about the classes that support both (simple tree?)

They are supported.
It's the second time I have faced this problem (check learner type). Is there a good way to do it?

What about constructing a dummy domain and using check_learner_adequacy?

astaric · 2016-04-18T08:18:36Z

I generally like the new hierarchy, although I cannot decide whether it should have and "abstract" class on top instead of the DontImpute imputer.

BTW, have you considered having a kind of a registry (https://github.com/biolab/orange3/blob/master/Orange/util.py#L105) to collect all available imputers (instead of hardcoding them in the widget)?

astaric · 2016-04-18T08:30:57Z

Orange/widgets/data/owimpute.py

+
+        button = self.default_button_group.button(self.MODEL_BASED_IMPUTER)
+        variable_button = self.variable_button_group.button(self.MODEL_BASED_IMPUTER)
+        if self.learner is None:


Currently, Model based imputer uses a default learner (simple tree), and the input is only used as a way to override the learner.

I can add default learner or add a new SimpleTree impute method.

I would prefer a default learner. This way, everything "just works", and if user wants to change the learner, he can.

s-alexey · 2016-04-19T10:29:02Z

Orange/widgets/data/owimpute.py

        self.data = data
+
        if data is not None:
            self.varmodel[:] = data.domain.variables


Is there a good way to filter variables without missed values?

Not that I know of.

astaric · 2016-05-09T07:15:53Z

Orange/preprocess/impute.py

+    format = "{var.name} -> {short_name}"
+    columns_only = False
+
+    def __init__(self, *args, **kwargs):


If init does nothing, you can ommit it.

astaric · 2016-05-09T07:39:14Z

The titles for sections Default method and Individual attribute settings are not displayed correctly on OSX.

astaric · 2016-05-09T07:41:25Z

Orange/widgets/data/owimpute.py

+                    method = self.variable_methods.get(i, self.default_method)
+
+                    if not method.check_supports_variable(var):
+                        self.warning(1, "Default method has ignored some variables.")


Default method could not impute some of the variables?

astaric · 2016-05-09T07:43:25Z

I have made some comments in the code. The only "major" issue I have found is the layout issue above. The rests seems ready for merge to me.

s-alexey · 2016-05-09T17:43:52Z

@astaric I have made some changes, can you review it and check the layout issue?

astaric · 2016-05-10T10:53:35Z

The layout issue remains. @kernc, can you check if it looks ok on your computer?

kernc · 2016-05-10T11:44:46Z

@astaric The layout seems ok here.

kernc · 2016-05-10T12:16:14Z

Orange/preprocess/impute.py

+    name = ""
+    short_name = ""
+    description = ""
+    format = "{var.name} -> {short_name}"


How about specifying this as "{var.name} → {self.short_name}". Then format_variable() method below could format it with self.format.format(var=var, self=self) and there would be no reason to override the method if format is already overriden?

Add BaseImpute class with base functions. Add variable type support checker. Refactor widget and add new gui features.

astaric · 2016-05-11T07:03:37Z

The box surrounding default methods/individual attribute settings is now (again) shown on OSX, so the layout issue is gone.

kernc · 2016-05-11T21:06:03Z

Orange/preprocess/impute.py

+class Average(BaseImputeMethod):
+    name = "Average/Most frequent"
+    short_name = "average"
+    description = "Replace with average/mode for the column"


average value / mode of. Sorry. :]

astaric · 2016-05-13T08:16:39Z

If @kernc has no more comments, I am going to merge this today. If you were planning on changing anything, please let me know.

CLAassistant · 2016-05-13T11:00:27Z

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ s-alexey
❌ Alexey Suharevich

Alexey Suharevich seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.

s-alexey · 2016-05-13T11:19:03Z

I'm not planning any changes.

Did not work properly since biolabgh-1094 Fixes biolabgh-1965

lanzagar added the gsoc label Mar 18, 2016

astaric assigned lanzagar Apr 1, 2016

lanzagar mentioned this pull request Apr 8, 2016

Impute widget crashes on learner input #1082

Closed

s-alexey force-pushed the iss1082 branch from 4e9bf83 to d3fe88d Compare April 16, 2016 20:01

s-alexey force-pushed the iss1082 branch 2 times, most recently from 58bec31 to ba6e3ac Compare April 16, 2016 20:40

astaric reviewed Apr 18, 2016
View reviewed changes

s-alexey force-pushed the iss1082 branch 4 times, most recently from 3df3cef to 310fdb0 Compare April 19, 2016 10:27

s-alexey reviewed Apr 19, 2016
View reviewed changes

s-alexey force-pushed the iss1082 branch from 310fdb0 to 74fce2e Compare April 19, 2016 11:02

lanzagar assigned astaric and unassigned lanzagar May 6, 2016

astaric reviewed May 9, 2016
View reviewed changes

s-alexey force-pushed the iss1082 branch from badfdac to 91f8309 Compare May 9, 2016 11:08

kernc reviewed May 10, 2016
View reviewed changes

s-alexey added 2 commits May 11, 2016 09:22

OWImpute: refactor widget and related module.

30ffb38

Add BaseImpute class with base functions. Add variable type support checker. Refactor widget and add new gui features.

Add impute tests

c3640cf

s-alexey force-pushed the iss1082 branch 3 times, most recently from 0ae879e to 1e8182d Compare May 11, 2016 06:57

s-alexey force-pushed the iss1082 branch 2 times, most recently from 1fba4a6 to 94ef41c Compare May 11, 2016 10:35

kernc reviewed May 11, 2016
View reviewed changes

s-alexey force-pushed the iss1082 branch from 94ef41c to 34ffe66 Compare May 13, 2016 11:00

OWImpute: provide default learner, fixup and cleanup.

abb7bd1

s-alexey force-pushed the iss1082 branch from 34ffe66 to abb7bd1 Compare May 13, 2016 11:07

astaric merged commit 4699f57 into biolab:master May 13, 2016

astaric mentioned this pull request May 13, 2016

[FIX] Impute widget: no longer crashes on learner inputs (issue #1082) #1122

Closed

s-alexey deleted the iss1082 branch July 13, 2016 18:57

ales-erjavec added a commit to ales-erjavec/orange3 that referenced this pull request Jan 31, 2017

owimpute: Fix per-variable value editing

9f64238

Did not work properly since biolabgh-1094 Fixes biolabgh-1965

Uh oh!

Conversation

s-alexey commented Mar 10, 2016

Uh oh!

codecov-io commented Mar 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current coverage is 86.15%

Uh oh!

astaric commented Apr 15, 2016

Uh oh!

s-alexey commented Apr 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

astaric commented Apr 18, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

astaric commented May 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

astaric commented May 9, 2016

Uh oh!

s-alexey commented May 9, 2016

Uh oh!

astaric commented May 10, 2016

Uh oh!

kernc commented May 10, 2016

Uh oh!

kernc May 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

astaric commented May 11, 2016

Uh oh!

kernc May 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

astaric commented May 13, 2016

Uh oh!

CLAassistant commented May 13, 2016

Uh oh!

s-alexey commented May 13, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov-io commented Mar 10, 2016 •

edited

Loading

kernc May 10, 2016 •

edited

Loading

kernc May 11, 2016 •

edited

Loading