8000 [MRG + 1] Add SVR mathematical description to the tutorial and a test for decision_function by artsobolev · Pull Request #4388 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

[MRG + 1] Add SVR mathematical description to the tutorial and a test for decision_function #4388

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Mar 18, 2015
Merged

[MRG + 1] Add SVR mathematical description to the tutorial and a test for decision_function #4388

merged 3 commits into from
Mar 18, 2015

Conversation

artsobolev
Copy link
Contributor

This PR adds SVR's description to the tutorial (Mathematical formulation section), adds a test to reproduce its decision_function (#4367), and makes sphinx use six that comes with sklearn being built, not the one already installed in a system.

artsobolev
artsobolev reviewed Mar 12, 2015
View reviewed changes
doc/conf.py
@@ -15,6 +15,9 @@
from __future__ import print_function
import sys
import os

sys.path.insert(0, os.path.abspath('..'))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me elaborate this: I have an old version of scikit-learn (something like 0.14), which ships very old version of six. Because project dir is not in the path (at least by the moment doc/sphinxext/gen_rst.py is called), the line below uses system's sklearn, and breaks building the doc. This doesn't look right to me.

Copy link
Member

Choose a reason for hiding this comment

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

I don't understand. You build the docs of the development system using the old version?
You should make sure that the version you are building the docs from is the one that gets imported.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@amueller, how can I do this?

I just ran make doc and it failed with an error of six module not having PY2 property. I inserted debug output (__version__ and __file__) into gen_rst.py, and it turned out, my system's sklearn was used. Can you try the same?

Copy link
Member

Choose a reason for hiding this comment

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

Many ways. For example you could use the ![develop mode][https://pythonhosted.org/setuptools/setuptools.html#develop-deploy-the-project-source-in-development-mode] or change your PYTHONPATH.
I don't have an "installed" version of scikit-learn, I only have the repo checkout ;)

@landscape-bot
Copy link

Code Health
Code quality remained the same when pulling e6542db on Barmaley-exe:svr-doc-n-test into 4a57c92 on scikit-learn:master.

amueller
amueller reviewed Mar 12, 2015
View reviewed changes
doc/modules/svm.rst
\textrm {subject to } & e^T (\alpha - \alpha^*) = 0\\
& 0 \leq \alpha_i, \alpha_i^* \leq C, i=1, ..., n

where :math:`e`, :math:`C > 0` and :math:`Q` are the same as in the case of SVC.
Copy link
Member

Choose a reason for hiding this comment

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

As the explanation for the variables is short, I'd copy and paste the explanation from SVC and say "as in the case of the SVC", so people don't have do dereference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, then, I think, we should also fix

Q is an n by n positive semidefinite matrix, Q_{ij} \equiv K(x_i, x_j) and \phi (x_i)^T \phi (x) is the kernel

because the kernel is a function of 2 arguments, and relation between Q and \phi is not clear.

@landscape-bot
Copy link

Code Health
Code quality remained the same when pulling eff77f5 on Barmaley-exe:svr-doc-n-test into 4a57c92 on scikit-learn:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0%) to 95.11% when pulling eff77f5 on Barmaley-exe:svr-doc-n-test into 4a57c92 on scikit-learn:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0%) to 95.11% when pulling eff77f5 on Barmaley-exe:svr-doc-n-test into 4a57c92 on scikit-learn:master.

@amueller amueller changed the title [MRG] Add SVR mathematical description to the tutorial and a test for decision_function [MRG + 1] Add SVR mathematical description to the tutorial and a test for decision_function Mar 16, 2015
@amueller
Copy link
Member

LGTM.
There was some confusion about whether intercept_ is actually rho, but I guess hat is somewhat tangential to this PR.

@ogrisel
Copy link
Member
ogrisel commented Mar 18, 2015

The fact that we have a decision_function method on a regression model looks like weird to me. I don't see what the "decision" is in the case of predicting the value of continuous variable(s). The computed values returned by the decision_function are always the same as for the predict method so this is redundant. The only difference is that the shape of the returned value of SVR.decision_functionis (n_samples, 1) instead of (n_samples,) for the SVR.predict method.

Apparently most linear regression models in sklearn (Rdige, SGDRegressor, LinearRegression, Lars, ElasticNet) also have a decision_function but they all have the same shape as predict, that is (n_samples,) for regular regression (n_outputs==1) or (n_samples, n_outputs) when multioutput is implemented and n_outputs > 1.

So I think we should:

  • at least make the shape consistent with the other models
  • maybe deprecate decision_function for regression model in favor of the predict method.

Both points should be tackled in dedicated PRs though.

WDYT?

@amueller
Copy link
Member

@ogrisel There was a discussion about having decision_function on regression models a couple of times, and it was confusing to me, too, see #1404. As @mblondel points out in #2588, this allows the use of ranking losses with regressors, though.
My recent standpoint was that we should instead add decision_function to all regressors. Then we can also common-test it and get the shapes right etc.
On the other hand, it seems a bit odd to have decision_function and predict do the same thing to hack around the scorer interface, so we could also try to fix that. The only way I see of doing this, is figuring out whether the estimator is a regressor or a classifier, which might also be useful in other areas.
We have is_classifier which relies on inheritance hierarchy, and we could use it here.
I'm not sure that our current is_classifier is the right thing to do (we claim we don't use class hierarchy).

ogrisel added a commit that referenced this pull request Mar 18, 2015
@ogrisel
Merge pull request #4388 from Barmaley-exe/svr-doc-n-test
defc0a9 
[MRG + 1] Add SVR mathematical description to the tutorial and a test for decision_function
@ogrisel ogrisel merged commit defc0a9 into scikit-learn:master Mar 18, 2015
@ogrisel
Copy link
Member
ogrisel commented Mar 18, 2015

Alright then. Thanks for the heads on the discussion. I just checked the HTML rendering of this branch and it looks good. The maths look correct to me from what I recall. +1 on my side as well and merging + backporting to 0.16.X.

@ogrisel
Copy link
Member
ogrisel commented Mar 18, 2015

@mblondel it would be great if you could quickly check the maths as well if you have 2min.

@mblondel
Copy link
Member
mblondel commented Mar 19, 2015 via email

@amueller
Copy link
Member
8000

I agree that we should decide to either rely on inheritance or define a tag like estimator_type to implement is_regressor and is_classifier properly.
Our current approach with is_classifier seems pretty half-assed to me.
I guess the easiest way out would be to define the tag in the base classes, so that everything has an estimator_type and if an external estimator doesn't, they can just define it. That would mirror what we do with get_params and set_params.
We don't have anything that is both a classifier and a regressor, right?
So it would be an exclusive tag that says what predict does, with possible values classifier, regressor and clusterer?
I'm sure @GaelVaroquaux has an opinion on this.
Maybe I'll implement that tomorrow....

@mblondel
Copy link
Member
mblondel commented Mar 19, 2015 via email

@amueller
Copy link
Member

That is what I meant.

@mblondel
Copy link
Member

+1 on my side

And in the scoring API, if estimator_type is not defined, we can resort to the current heuristic.

@amueller amueller mentioned this pull request Mar 23, 2015
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@artsobolev @landscape-bot @coveralls @amueller @ogrisel @mblondel
0