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

[ENH] Add all available methods to feature selection preprocessor #2205

Merged
merged 2 commits into from
Apr 28, 2017

Conversation

lopezco
Copy link
Contributor

@lopezco lopezco commented Apr 7, 2017

Issue

Add missing feature selection methods to OWPreprocess

Includes
  • Code changes
  • Tests
  • Documentation

@CLAassistant
Copy link

CLAassistant commented Apr 7, 2017

CLA assistant check
All committers have signed the CLA.

@codecov-io
Copy link

codecov-io commented Apr 7, 2017

Codecov Report

Merging #2205 into master will increase coverage by 0.96%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2205      +/-   ##
==========================================
+ Coverage   71.06%   72.02%   +0.96%     
==========================================
  Files         318      318              
  Lines       54597    54597              
==========================================
+ Hits        38797    39325     +528     
+ Misses      15800    15272     -528

@astaric
Copy link
Member

astaric commented Apr 24, 2017

@janezd looks good to me. What do you think?

{"text": "ANOVA"},
{"text": "Chi2"},
{"text": "RReliefF"},
{"text": "UnivariateLinearRegression"}
Copy link
Contributor

Choose a reason for hiding this comment

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

Use spaces: this is shown in a combo.

@janezd
Copy link
Contributor

janezd commented Apr 27, 2017

I'm not opposed to merging, but I'd be much happier if error messages were improved.

Loading heart_disease and selecting Univariate linear regression gives an error "Scoring method WrapperMeta requires a class variable of type ContinuousVariable". The end-user should be given messages from the scripting-level exceptions only as a last resort. Here, the user doesn't know what is ContinuousVariable and, especially, what is WrapperMeta. This error is not much better than a crash.

To be fair, the existing scorers also refer to DiscreteVariable. But at least they don't have WrapperMeta. The problem is that the additional scorers seem less robust, so the user get more cryptic messages. Running, for instance, ANOVA on heart_disease gives "Only ContinuousVariables are supported", without explaining that this holds only for ANOVA.

The question (@astaric, @BlazZupan) is whether we allow this widget to be somewhat unfriendly (knowing that this PR is not the cause, it just increases it by some amount) or do we, for instance, add checks before calling feature selectors and issue friendly error messages instead of showing exception messages.

@astaric
Copy link
Member

astaric commented Apr 28, 2017

@janezd, Cryptic error messages should also be changed in the scripting part, which is IMHO not a part of this PR. (Hearing about WrapperMeta is never useful)

I'll go ahead and merge this and we can improve error messages as a part of another PR.

@astaric astaric merged commit 8e2f09b into biolab:master Apr 28, 2017
@janezd
Copy link
Contributor

janezd commented Apr 28, 2017

Agreed. I'm assigning this to myself.

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.

5 participants