-
Notifications
You must be signed in to change notification settings - Fork 27
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
Regression results part2 #28
Changes from 5 commits
54e8aa0
9d8e5bf
c378361
b12fe8a
f3b7d94
b3e11d6
5cef948
88ac939
be4a07e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,7 @@ | |
# The full license is in the file COPYING.txt, distributed with this software. | ||
# ---------------------------------------------------------------------------- | ||
import pandas as pd | ||
from skbio.stats.composition import ilr_inv | ||
|
||
|
||
class RegressionResults(): | ||
|
@@ -57,3 +58,72 @@ def __init__(self, stat_results, | |
# calculate the overall coefficient of determination (i.e. R2) | ||
sst = sse + ssr | ||
self.r2 = 1 - sse / sst | ||
|
||
def _check_projection(self, project): | ||
""" | ||
Parameters | ||
---------- | ||
project : bool | ||
Specifies if a projection into the Aitchison simplex can be | ||
performed. | ||
|
||
Raises | ||
------ | ||
ValueError: | ||
Cannot perform projection into Aitchison simplex if `basis` | ||
is not specified. | ||
ValueError: | ||
Cannot perform projection into Aitchison simplex | ||
if `feature_names` is not specified. | ||
""" | ||
if self.basis is None and project: | ||
raise ValueError("Cannot perform projection into Aitchison simplex" | ||
"if `basis` is not specified.") | ||
|
||
if self.feature_names is None and project: | ||
raise ValueError("Cannot perform projection into Aitchison simplex" | ||
"if `feature_names` is not specified.") | ||
|
||
def coefficients(self, project=False): | ||
""" Returns coefficients from fit. | ||
|
||
Parameters | ||
---------- | ||
project : bool, optional | ||
Specifies if coefficients should be projected back into | ||
the Aitchison simplex. If false, the coefficients will be | ||
represented as balances (default: False). | ||
|
||
Returns | ||
------- | ||
pd.DataFrame | ||
A table of values where columns are coefficients, and the index | ||
is either balances or proportions, depending on the value of | ||
`project`. | ||
|
||
Raises | ||
------ | ||
ValueError: | ||
Cannot perform projection into Aitchison simplex if `basis` | ||
is not specified. | ||
ValueError: | ||
Cannot perform projection into Aitchison simplex | ||
if `feature_names` is not specified. | ||
""" | ||
self._check_projection(project) | ||
coef = pd.DataFrame() | ||
|
||
for r in self.results: | ||
c = r.params | ||
c.name = r.model.endog_names | ||
coef = coef.append(c) | ||
|
||
if project: | ||
# `check=True` due to type issue resolved here | ||
# https://github.com/biocore/scikit-bio/pull/1396 | ||
c = ilr_inv(coef.values.T, basis=self.basis, check=False).T | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. check=True in the comment but False in the code, which one is it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the reason why I added this comment. Although this has been resolved in scikit-bio, it is only in the development version - which would mean that this would have to depend on the development version of scikit-bio. I was thinking about raising an issue, and making the fix at the next release of scikit-bio. What do you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That fix has been merged in skbio and you could use the dev version of skbio (in travis, setup.py, etc) until there is release, will this work? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok. Then this should be ok On Jul 31, 2016 6:38 AM, "Antonio Gonzalez" notifications@github.com
|
||
c = pd.DataFrame(c, index=self.feature_names, | ||
columns=coef.columns) | ||
return c | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what about: return pd.DataFrame(c, index=self.feature_names, columns=coef.columns) ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
else: | ||
return coef |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you planning to use this check in other methods? If not, why not add it in the coefficients code (it looks like a really simple check that is happening within
coefficients
)?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main reason is because a whole bunch of methods are using this check, namely
residuals
andpredict
. Those are being added in #30There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, thanks. Just out of curiosity, will it make sense to make project a member of the object? What I'm asking is if it will make sense to do somewhere in the code: self.project=True/False, and then just call self.coefficients() & self._check_projection(). This could simplify the code but it will mean that if you are doing self.residuals, self.predict, etc this parameter can't change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, @mortonjt and I discussed this offline and it makes sense to keep as is. Basically, this is converting the results to one of two formats based on the projection.