Skip to content

Add CVRMSE (Coefficient of variation of the RMSE)#2209

Closed
lopezco wants to merge 4 commits intobiolab:masterfrom
lopezco:patch-3
Closed

Add CVRMSE (Coefficient of variation of the RMSE)#2209
lopezco wants to merge 4 commits intobiolab:masterfrom
lopezco:patch-3

Conversation

@lopezco
Copy link
Copy Markdown
Contributor

@lopezco lopezco commented Apr 7, 2017

Description of changes

Scoring metric for regression

Includes
  • Code changes
  • Tests
  • Documentation

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Apr 7, 2017

CLA assistant check
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.

✅ lopezco
❌ Jose LOPEZ


Jose LOPEZ 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.
You have signed the CLA already but the status is still pending? Let us recheck it.

@codecov-io
Copy link
Copy Markdown

codecov-io commented Apr 7, 2017

Codecov Report

Merging #2209 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2209      +/-   ##
==========================================
+ Coverage   71.12%   71.13%   +<.01%     
==========================================
  Files         318      318              
  Lines       54668    54671       +3     
==========================================
+ Hits        38885    38888       +3     
  Misses      15783    15783

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d2e8901...3d05503. Read the comment docs.

Copy link
Copy Markdown
Contributor

@janezd janezd left a comment

Choose a reason for hiding this comment

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

There are zillion of performance measures and I'd be hesitant to burden Orange, and widgets in particular, with all of them. This one looks sensible at first - it essentially gives the percentage of error. On the second look, it's maybe not: if a classifier misses my age by 10 %, I'd recalculate this to actual years to get a grasp of the error size. Wouldn't the user always know the range and be interested in the absolute error?

Another score that we could add is the sum of squares of [(y - y_predicted) / y] over all y. And we could probably come up with many more...

@BlazZupan, @astaric, @ajdapretnar: your opinions? Do we reorganize the widget to eventually support dozens of scores or do we stay with the essentials?

Coefficient of variation of the RMSE
"""
def compute_score(self, results):
return RMSE(results)/np.nanmean(results.actual) * 100
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Spaces around /.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What if np.nanmean(results.actual) is zero (or very small) or negative?


class CVRMSE(Score):
"""
Coefficient of variation of the RMSE
Copy link
Copy Markdown
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 better to call this normalized RMSE? I know it is ambiguous, but CVRMSE doesn't tell me anything.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I like the idea of adding more (not dozens maybe) scores. We had some requests like sensitivity and selectivity from scientists who are interested in using the infrared addon. Also, it would be nice, if the scores are displayed in an easily copy-pasteable table...

@ajdapretnar
Copy link
Copy Markdown
Contributor

Perhaps it's a stupid idea, but could we create an add-on, that is essentially Orange on steroids? Which would enhance Orange with all sorts of measures and features, but we would keep the core distro light?
I agree with @janezd that introducing everything but the kitchen sink to Orange might be against our development philosophy (keep it easy and user friendly).
However, as @lopezco and @borondics noticed, sometimes more advanced users would like extra features for their analysis.
Is there a way to have the cake and eat it, too? 😄

@lopezco
Copy link
Copy Markdown
Contributor Author

lopezco commented Apr 14, 2017

Is it possible to add another input to test and score (like python function) to define custom metrics?

This way users could define the metrics that they want to evaluate.

@astaric
Copy link
Copy Markdown
Member

astaric commented Apr 14, 2017

+1 for additional input that accepts additional scorers. We already have this in Rank and Preprocess widgets, I cannot think of a reason not to add it to Test & Score. @janezd?

@janezd
Copy link
Copy Markdown
Contributor

janezd commented Apr 15, 2017

I'm not too happy with the "powerpack" idea. It's great on the surface, but in the future it can lead to a "kitchen sink add on", and to a confusion about what is in the core and what is available only in the add-on. Not to mention that a user who would need (or like, but not necessarily need) a single extra feature would have to install the entire kitchen sink and pollute Orange with dozens of in-the-rare-case-that-anybody-would-be-interested-in-this features. I so don't like where this could be going; I'd like to have a more thorough discussion before doing this.

As for the new signal, I'm skeptical of small, single-use signals for specific widgets. I believe signals should be general, and we shouldn't have too many different types. Specific signals don't make widgets reusable. It's like lego bricks that fit only on a single car model. Lego shouldn't have gone there.

Considering the above two options, I'm more inclined towards the third one, which I disliked the first (but still the least): redesign the Test & Score widget to be able to accommodate a larger number of scorers without looking like a kitchen sink.

@borondics
Copy link
Copy Markdown
Member

I completely see the "kitchen sink" point and agree with not adding dozens of features.

I like the third option of @janezd : maybe add scoring features and make a little icon on the widget itself that would allow the user to select which ones they want to display - then it is up to the individuals how much visual pollution they can bear with?

@lopezco
Copy link
Copy Markdown
Contributor Author

lopezco commented Apr 15, 2017

But the idea of having an extra input with a custom function can be done by connecting a OWPythoScript with a metric function on one of its outputs... For instance out_object.

@janezd
Copy link
Copy Markdown
Contributor

janezd commented Apr 17, 2017

Even if we use PythonScript for this, Test&Score would still need to accept an additional input. In this way, we can also add additional inputs of many different types to many widgets and let the users use PythonScript to provide the instances of this types. I don't like this path.

But I have put redesigning the Test & Score on my agenda for this Friday. I'll try to find a non user-intimidating, but clean way to include dozens of scores into the widget.

@janezd
Copy link
Copy Markdown
Contributor

janezd commented Apr 28, 2017

We can now decide about this.

CVRMSE can be added since #2257 ensures that the user doesn't see too many columns. On the other hand, it can be a part of add-on (which?!) since #2269 will pull all existing scorers into the Test & Score widget.

In either case, this PR is incompatible with #2269: changing owtestlearners.py is no longer necessary (and will cause a conflict). Besides, it has to define the class_types class attribute. This PR can still be merged and the conflict later resolved in #2269.

@janezd
Copy link
Copy Markdown
Contributor

janezd commented Apr 29, 2017

I moved this code (in a slightly adapted form) to #2269. Thanks!

@janezd janezd closed this Apr 29, 2017
@lopezco lopezco deleted the patch-3 branch April 30, 2017 08:29
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.

7 participants