Merge learner widgets

[pavlin-policar]

A much simpler approach to handling data with continuous or discrete target variables inside learner widgets.

The BaseLearner now defines a problem_type context setting that tells us whether the data represents a classification or regression problem. This setting can be manually set with radio buttons if there is no data (this is required to work properly with the Test&Score widget). If there is data on the input, we disable the radio buttons and assign the appropriate type. The radio buttons are completely opt-in and can be enabled with a flag on the widget, so widgets that only handle one kind of target variable don't have to deal with any extra logic.

Each individual learner can then override the on_type_change method, where they can do whatever updates they need to the UI or anything else.

I've made the modifications necessary for this approach to work to the TreeLearner widget. Please let me know if this is an okay approach and if I should continue this route and port the other widgets to something like this.

[janezd]

This was an example of bad communication: tree classifier and regressor were already merged into a single widget, but unfortunately within a PR that was neglected from July. Sorry about that... The PR was finally merged today; please update to today's master and see the regression tree widget.

[janezd]

@BlazZupan, will the merged category (classify + regression) be called "learners"?

• We have "Unsupervised", so to keep in line with that the new category would have to be "Supervised". It's weird, I don't like it.
• A better option is "Modelling". The form doesn't fit, but we already have a mess -- "Visualize", "Classify" and "Regression", and not "Visualization", "Classification" and "Regression" (or "Visualize", "Classify" and "Regress"!?).
• The problem with "Modelling" is that it also covers unsupervised modelling.

Still, I vote for "Model" or "Modelling".

[janezd]

@BlazZupan agrees with Model as the category name.

[janezd]

Yes. This is "forced commit"; the widget needs to set the output when constructed or upon receiving new data even if auto-commit is off.

[janezd]

I don't like the radio buttons. Besides using space, they unnecessarily burden the user. Say the the user connect a File widget and a learner widget to Test Learners. From the user's perspective, the learner widget outputs a "modelling algorithm". The algorithm "should be able to handle classification and regression data". Having to specify the kind of data seems "unreasonable", since the algorithm "sees" what kind of data it got.

The philosophy is that the user can decide how the algorithm should run and not that is has to set the algorithm properly to run for a particular data type (when the decision is trivial and can be automated).

The need for the radio buttons hence doesn't come because the user needs to decide something but just because classifier and regressor are implemented as separate classes and Test Learners needs to be given the correct class.

I suggest two solutions.

• Ideally, learning algorithms should be able to handle both regression and classification, in particular if they share most of the code, such as in knn.
• Otherwise, we can have a general dispatcher for learning. It would be derived from Fitter, and contain a classifier and regressor, e.g. fitter = FitterDispatcher(tree_classifier, tree_regressor), and then it would self.send('Learner', fitter) to Test and Score. The dispatcher would then delegate the learning to either classifier or regressor, depending on the data type.

[pavlin-policar]

I agree, this would be a much better approach.

But merging the learners into a single one still leaves a problem. While some of the learners (KNN, AdaBoost, RF) can be easily merged since they their UIs are identical, others take different parameters that need to be set in the UI.

• Linear regression supports none, L1, L2 and elastic net regularization whereas logistic regression only supports L1 and L2 regularization.
• In the new tree widgets, the classification has an extra option to stop when the majority reaches a certain percent while the regression one does not have this option.
• The SVM can output either a C-SVM model for classification or an epsilon-SVM for regression.

Of course it makes sense that regression and classification problems can have slightly different parameters, but if we merge them into single learners where should these problem specific parameters appear or should they disappear altogether? Should the parameters be renamed to something more generic so they refer to either problems (but that might lead to confusion)?

If the learner widget has an input table, then displaying the correct options is simple and requires no input from the user, but if we just connect them to the Test&Score widget, how would the user be able adjust the specific parameters for instance regression since the learner widget has no awareness of the data (which makes sense)? This was the reasoning behind the radio buttons, since I haven't been able to come up with a better solution other than removing the specific parameters altogether.

As an aside, I've noticed that the SGDClassifier can be turned into logistic regression and does support all the types of regularization that linear regression does so this may not be a problem at all since then they could have the same parameters.

[janezd]

Huh, I see. I'd say that it two methods (classification/regression) require substantially different GUIs that cannot be nicely merged into one, they should remain separate widgets. An extra option is not a problem, even a few options in separate (always visible?) boxes for classification and regression are OK, IMHO.

However, we will always have pure classification or regression methods (CN2?), and we could even keep logistic and linear regression in separate widgets if we had to.

[pavlin-policar]

I do think that options that are always visible could be confusing and not the most user friendly, since when I change an option I expect a change, but that may not always happen in this case. Maybe help text, tooltips, help icons, ...

Anyway, I'll get on merging the ones that make sense first.

[kernc]

Could remain visible but become disabled.

[janezd]

@kernc, read more carefully. Disabled based upon ... what?

[kernc]

Input, provided provided.

[janezd]

I'm wasting time, I guess, but I'll bite. :)

If the learner widget has an input table, then displaying the correct options is simple and requires no input from the user, but if we just connect them to the Test&Score widget, how would the user be able adjust the specific parameters for instance regression since the learner widget has no awareness of the data (which makes sense)?

[kernc]

Right.

[codecov-io]

Current coverage is 89.03% (diff: 100%)

Merging #1657 into master will increase coverage by 0.06%

@@             master      #1657   diff @@
==========================================
  Files            82         85     +3   
  Lines          8963       9019    +56   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           7974       8030    +56   
  Misses          989        989          
  Partials          0          0          

Powered by Codecov. Last update 11c3f11...058dc41

[pavlin-policar]

What might be a good solution when there are different options in the UI in dropdown menus that are specific to classification and regression problems?

For example, AdaBoost is identical in all aspects except the algorithm and should definitely be merged, but since the learner is completely unaware of the data (until it gets some data, but that doesn't matter since it has to work with Test&Score).

I'm not sure how I would go about solving this.

[janezd]

I would have boxes "Discrete target (Classification)" and "Numeric target (Regression)", and put the specific controls into them. (I may change my mind about the box names, but at the moment I like them. :)

While you're changing the interface, could you also tidy it up a bit? Make the spin boxes the same width; see the classification tree widget for an example. I guess the two combo boxes should also have the same width (but not the same as the (narrower) spins).

[janezd]

This changes the order of preprocessors. I guess you shouldn't do this.

[janezd]

OWKNNLearner = OWKNNBase

[janezd]

Typo.

[pavlin-policar]

@janezd After looking over what we discussed more carefully, there should actually not be any need to set the domain on the learner inside the fitter __call__ method, since each individual learner sets its own property in their own __call__ method. It's actually the fitter instance that remains without the domain, which should be fine, since it doesn't need it and shouldn't need it.

[janezd]

@astaric, we're seeing more and more of this checking whether a class has overrides an inherited method. (Not that it's nice, but it's useful.) Is it time to implement something like

def overrides(method, base):
    return getattr(type(method.__self__), method.__name__) != getattr(base, method.__name__)

to be used like (here above) if overrides(self.add_classification_layout, OWBaseLearner)?

[janezd]

I see, of course.

It looks safe to merge, unless @kernc has any objections. Just thing: can you remove the change to ada_boost, so the pr doesn't touch more files than necessary?

[pavlin-policar]

Sure thing.

[kernc]

Looks good. Sorry for being so pedantic. Best start with a tidy core this time around; it spoils soon enough anyway.

[kernc]

Looks good. Sorry for being so pedantic. Best start with a tidy core this time around; it spoils soon enough anyway.

[kernc]

You can elegantly avoid the static method below if you use a more verbose variable name:

def __kwargs(self, problem_type):
    learner_kwargs = set(self.__fits__[problem_type].__init__.__code__.co_varnames[1:])
    return {k: v for k, v in self.kwargs.items() if k in learner_kwargs)

You also needn't pass kwargs separately.

[kernc]

Is this more an AttributeError or a TypeError?

[kernc]

I still don't think checking name attribute is adequate. e6a14f0 in #1771 obsoletes redundant short names on Learner types. Can you instead check that the class' name equals 'Fitter' or that none of the bases is named 'Fitter' or similar?

Also, kwargs is actually a dict of class attributes, so attrs or similar might be more appropriate.