Skip to content
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

[FIX] owlearnerwidget: Fix output initialization #1562

Merged
merged 3 commits into from
Oct 1, 2016

Conversation

ales-erjavec
Copy link
Contributor

Always output the initial learner, disregarding the auto_apply.

Always output the initial learner, disregarding the `auto_apply`.
Assert that initialized widgets have a valid learner output
@ales-erjavec ales-erjavec force-pushed the base-learner-auto-commit-fix branch from 87ce67e to 5c6b60a Compare September 16, 2016 08:51
@codecov-io
Copy link

codecov-io commented Sep 16, 2016

Current coverage is 88.66% (diff: 100%)

Merging #1562 into master will decrease coverage by 0.19%

@@             master      #1562   diff @@
==========================================
  Files            78         78          
  Lines          8099       8124    +25   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           7197       7203     +6   
- Misses          902        921    +19   
  Partials          0          0          

Sunburst

Powered by Codecov. Last update abbf69f...b24d455

@@ -123,7 +123,7 @@ def __init__(self):
self.preprocessors = None
self.outdated_settings = False
self.setup_layout()
QTimer.singleShot(0, self.apply)
QTimer.singleShot(0, self.update_learner)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be safer to call self.unconditional_apply here? It doesn't change anything for the base class since update_model doesn't do anything without the data, but derived classes can add some code to the apply method or change update_model to do something without data.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did try that. At the least that breaks 'Linear Regression' widget because it overrides the add_bottom_buttons method and does not 'define' unconditional_apply so it fails to even initialize.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Still, I think it's too dangerous to skip the potentially overridden apply. It can lead to a situation in which the widget will, for instance, ignore some setting when force-applied.

Let's require all widgets derived from OWBaseLearner to have unconditional_apply. All that don't, need to be fixed. Linear regression, in particular, doesn't need to define commit at all (partially my fault :()

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ales-erjavec, @astaric, please see #1601, which should also fix this problem. Two other widgets in education remove buttons, while (AFAIK) no widgets in core, bio, datafusion, network, prototypes or text extend BaseLearner.

What about replacing the above line with
QTimer.singleShot(0, getattr(self, "unconditional_apply", self.apply))?

If the widget does not have unconditional_apply it has either left apply as it is, hence calling it has the same effect as unconditional_apply would, or it has overridden apply, so we must call it anyway. If the widget creates its own Apply button with a callback with a different name, then it will have to reimplement the output initialization, too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'this problem' = the problem with linear regression, not the problem fixed by this PR here.

@janezd janezd merged commit 2ec19df into biolab:master Oct 1, 2016
@ales-erjavec ales-erjavec deleted the base-learner-auto-commit-fix branch October 11, 2016 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants