-
Notifications
You must be signed in to change notification settings - Fork 191
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
Correct R² formulas and add Efron's R² #400
Conversation
This PR fixed the R² formulas, which were previously based on the deviance instead of the likelihood function. It also adds Efron's R². The following variants were benchmarked against Stata's fitstat: McFadden, Cox and Snell, Nagelkerke and Efron. Cox and Snell's and Efron's R² should match the classical R² for linear models.
Codecov Report
@@ Coverage Diff @@
## master #400 +/- ##
==========================================
- Coverage 97.97% 97.89% -0.08%
==========================================
Files 17 16 -1
Lines 1282 1281 -1
==========================================
- Hits 1256 1254 -2
- Misses 26 27 +1
Continue to review full report at Codecov.
|
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.
Thanks. Sorry for the delay, I had missed the PR.
src/statmodels.jl
Outdated
@@ -197,7 +197,7 @@ Coefficient of determination (R-squared). | |||
For a linear model, the R² is defined as ``ESS/TSS``, with ``ESS`` the explained sum of squares | |||
and ``TSS`` the total sum of squares. | |||
""" | |||
r2(obj::StatisticalModel) = mss(obj) / deviance(obj) | |||
r2(obj::StatisticalModel) = mss(obj) / sum(abs2, response(obj) .- meanresponse(obj)) |
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.
Better do μ = meanresponse(obj); sum(x -> abs2(x - μ), response(obj))
to avoid allocating a copy. Maybe turn this into a long-form function for readability.
@Nosferican Is this formula OK for your package?
EDIT: actually I think we'd better deprecate this function and leave packages to implement it for linear models. That will give a clearer error message when called on a nonlinear model (instead of an error about mss
not being defined).
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.
avoid allocating a copy
I thought that sum
didn't allocate transform scalars into vectors with the dot syntax.
we'd better deprecate this function and leave packages to implement it for linear models
Do you mean removing the function and leaving a warning or leaving the function and adding a warning?
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.
Consider this script,
using BenchmarkTools
a(x, y) = sum(abs2, x - y)
b(x, y) = sum(abs2, x .- y)
c(x, y) = sum(xy -> abs2(xy[1] - xy[2]), zip(x, y))
d(x, y) = sum(abs2(a - b) for (a, b) ∈ zip(x, y))
using Random: seed!
seed!(0)
const x = rand(1000);
const y = rand(1000);
@btime a($x, $y)
@btime b($x, $y)
@btime c($x, $y)
@btime d($x, $y)
703.416 ns (1 allocation: 7.94 KiB)
659.205 ns (1 allocation: 7.94 KiB)
823.250 ns (1 allocation: 32 bytes)
969.684 ns (2 allocations: 48 bytes)
It seems to be trivially faster, but it certainly allocates as opposed to c
or d
.
If the R²
matches Stata, it will match mine. Some models will still need to define the method for some panel estimators, but this seems good.
If the statsmodels functions are being moved to StatsModels it might be best to make the decision there (better to trim there, than to have lost useful code then),
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.
I thought that sum didn't allocate transform scalars into vectors with the dot syntax.
As @Nosferican shows, it allocates, and it will probably stay that way in the future. Maybe another syntax will be introduced to do that.
Do you mean removing the function and leaving a warning or leaving the function and adding a warning?
I mean print a warning using Base.depwarn(..., :r2)
, saying that packages should define their custom method. Then we'll turn the warning into an error just like for other function stubs in the file.
If the R² matches Stata, it will match mine. Some models will still need to define the method for some panel estimators, but this seems good.
OK, cool.
If the statsmodels functions are being moved to StatsModels it might be best to make the decision there (better to trim there, than to have lost useful code then),
It's not clear yet what will happen and when, and given how small this function is that's really not an issue.
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.
I've added a deprecation warning. Let me know if it's okay.
src/statmodels.jl
Outdated
y = response(obj) | ||
ŷ = fitted(obj) | ||
μ = meanresponse(obj) | ||
1 - sum(abs2, y .- ŷ) / sum(abs2, y .- μ) |
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.
Same remark as above about avoiding allocations.
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.
check
src/statmodels.jl
Outdated
- `:MacFadden` (a.k.a. likelihood ratio index), defined as ``1 - \\log (L)/\\log (L_0)``; | ||
- `:CoxSnell`, defined as ``1 - (L_0/L)^{2/n}``; | ||
- `:Nagelkerke`, defined as ``(1 - (L_0/L)^{2/n})/(1 - L_0^{2/n})``; | ||
- `:Efron`, defined as ``1 - \\sum_i (y_i - \\hat{y})^2 / \\sum_i (y_i - \\bar{y})^2``. |
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.
Should be \\har{y}_i
.
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.
check
src/statmodels.jl
Outdated
1 - exp(2/nobs(obj) * (ll0 - ll)) | ||
elseif variant == :Nagelkerke | ||
(1 - exp(2/nobs(obj) * (ll0 - ll)))/(1 - exp(2/nobs(obj) * ll0)) | ||
# The following variants were benchmarked against Stata's fitstat: |
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.
I'd rather put this mention in the tests next to the expected values. The explanation about linear models can go to the docstring.
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.
Tests in GLM.jl?
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.
Some tests here are run using GLM which uses this package... Might be best test these eventually without another package for it.
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.
Yeah, in GLM.jl (since the check against Stata applies to GLMs AFAICT).
It's not ideal to have this split in two packages, but testing things directly here implies writing pseudo-model types. That's not so hard, but it will take some work.
src/statmodels.jl
Outdated
@@ -259,10 +271,9 @@ In this formula, ``L`` is the likelihood of the model, ``L0`` that of the null m | |||
of the model (as returned by [`dof`](@ref)). | |||
""" | |||
function adjr2(obj::StatisticalModel, variant::Symbol) | |||
ll = -deviance(obj)/2 | |||
ll0 = -nulldeviance(obj)/2 | |||
ll = likelihood(obj) |
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.
AFAICT you want loglikelihood
here. The fact it wasn't caught by testing is a bit worrying: isn't this covered by GLM's tests?
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.
Sorry. You're right. I hadn't tested the adjusted R².
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. Do you confirm it works know?
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.
Yes, sir!
src/statmodels.jl
Outdated
y = response(obj) | ||
ŷ = fitted(obj) | ||
μ = meanresponse(obj) | ||
1 - sum(abs2, y .- ŷ) / sum(x -> abs2(x - μ), response(obj)) |
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.
sum(abs2, y .- ŷ)
also needs to be changed.
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.
check
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.
Looks good, thanks!
src/statmodels.jl
Outdated
"Packages should define their own methods.", :r2) | ||
|
||
μ = meanresponse(obj) | ||
mss(obj) / sum(x -> abs2(x - μ), response(obj)) |
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.
Shouldn't these take into accounts weights since weights would have been handled in mss
and deviance
?
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.
Good catch. That also applies to the Efron R². For the method above, since it's deprecated, we'd better keep using deviance
for now. For the Efron R², I guess we need to multiply observations by their weights, but will it work for all kinds of models?
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.
Precisely: packages can handle weights through mss(obj)
and loglikelihood(obj)
. There's no need to provide for weights here.
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.
Not sure for ProbabilityWeights
, but should work fine for the other weights supported if not mistaken. The tests should cover the weight cases for sure.
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.
Efron's R² does need special treatment, since it uses the sum of residuals.
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.
Do you know the formula for weighted Efron R²? I guess we just need to multiply both the numerator and the denominator?
I think so. How are we going to implement it? We can retrieve weights via weights
, but how do we check if weights were defined in the first place?
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.
I think the default is just a one array. Might be nice to follow FillArrays.jl for it to implement the UnitWeight or something.
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.
Let's call weights
and assume it returns a valid AbstractVector
. Then it would make sense to define UnitWeights
in this package, to complement other kinds of weights (#135).
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.
Bump. Can you drop the Efron R² for now so that we can merge the PR? We can discuss improvements later.
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.
Since #515 we have UnitWeights
, so we can require models to return this when they are unweighted.
Thanks! Can you file a PR against GLM to update R² tests? |
As far as I can tell, the only tests involve linear models. This PR shouldn't affect them. |
OK. Then it would be nice to add tests to ensure results remain correct in the future. ;-) |
Since #400 we use the log-likelihood.
Since #400 we use the log-likelihood.
This PR fixed the R² formulas, which were previously based on the deviance instead of the likelihood function. It also adds Efron's R².
The following variants were benchmarked against Stata's fitstat: McFadden, Cox and Snell, Nagelkerke and Efron.
Cox and Snell's and Efron's R² should match the classical R² for linear models.
Closes #396.