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

Add keyword arg to modelmatrix; define momentmatrix #16

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

gragusa
Copy link

@gragusa gragusa commented Jun 15, 2022

  1. The modelmatrix has now a keyword weighted=false which is useful for dealing with weighted models.
  2. Add momentmatrix - this function is intended to return the matrix of estimating equations; for instance, for a linear model should return u*X, where u is the vector of residuals and X is the model matrix.

src/regressionmodel.jl Outdated Show resolved Hide resolved
src/statisticalmodel.jl Outdated Show resolved Hide resolved
src/regressionmodel.jl Outdated Show resolved Hide resolved
src/regressionmodel.jl Outdated Show resolved Hide resolved
src/statisticalmodel.jl Outdated Show resolved Hide resolved
test/regressionmodel.jl Outdated Show resolved Hide resolved
@@ -6,13 +6,32 @@ using StatsAPI: RegressionModel, crossmodelmatrix
struct MyRegressionModel <: RegressionModel
end

struct ItsRegressionModel <: RegressionModel
wts
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
wts
wts::AbstractVector

src/statisticalmodel.jl Show resolved Hide resolved
test/statisticalmodel.jl Outdated Show resolved Hide resolved
src/regressionmodel.jl Outdated Show resolved Hide resolved
gragusa and others added 7 commits June 17, 2022 00:34
Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>
Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>
Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>
Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>
@codecov-commenter
Copy link

codecov-commenter commented Jun 16, 2022

Codecov Report

Attention: Patch coverage is 75.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 97.43%. Comparing base (20b38e1) to head (93f8742).
Report is 9 commits behind head on main.

Files with missing lines Patch % Lines
src/regressionmodel.jl 75.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##              main      #16      +/-   ##
===========================================
- Coverage   100.00%   97.43%   -2.57%     
===========================================
  Files            3        2       -1     
  Lines           37       39       +2     
===========================================
+ Hits            37       38       +1     
- Misses           0        1       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


Return the model matrix (a.k.a. the design matrix).
Return the model matrix (a.k.a. the design matrix) or, if `weighted=true` the weighted
model matrix, i.e. `X' * sqrt.(W)`, where `X` is the model matrix and
Copy link
Member

Choose a reason for hiding this comment

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

Why transpose X? It sounds weird to change the orientation of the result depending on whether it's weighted or not.

Copy link
Author

Choose a reason for hiding this comment

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

My bad...I will fix it

src/regressionmodel.jl Outdated Show resolved Hide resolved

Return the model matrix (a.k.a. the design matrix).
Return the model matrix (a.k.a. the design matrix) or, if `weighted=true` the weighted
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Return the model matrix (a.k.a. the design matrix) or, if `weighted=true` the weighted
Return the model matrix (design matrix) or, if `weighted=true` the weighted


Return `X'X` where `X` is the model matrix of `model`.
Return `X'X` where `X` is the model matrix of `model` or, if `weighted=true`, `X'WX`,
where `W` is the diagonal matrix whose elements are the model weights.
Copy link
Member

Choose a reason for hiding this comment

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

Can we define weights?

Copy link
Member

Choose a reason for hiding this comment

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

We could add a link to the weights(::StatisticalModel) method. Indeed there can be confusion between prior weights and working weights (though these terms can also confuse casual users).

Copy link
Author

Choose a reason for hiding this comment

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

How exactly do I add a link to weights(::StatisticalModel)? Is there a way to link docs from different packages?

Copy link
Member

Choose a reason for hiding this comment

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

It's in the same package so I think something like [model weights](@ref weights(::StatisticalModel)) should work. Better test it though by building the StatsBase docs (julia docs/make.jl) using the updated StatsAPI.

src/statisticalmodel.jl Outdated Show resolved Hide resolved
gragusa and others added 3 commits June 17, 2022 14:47
Co-authored-by: Moritz Schauer <moritzschauer@web.de>
@gragusa
Copy link
Author

gragusa commented Jun 21, 2022

The only thing that we probably should do is to allow for modelmatrix to take another keyword argument, e.g., droppcollinearcols, to return only the columns corresponding to non-NaN coefficients in a Pivot Cholesky.

We could do it next (after I drop a bomb-PR against GLM. The GLM PR is waiting for this PR to get merged)

@nalimilan
Copy link
Member

Let's tackle this separately. :-)

I'd rather review the GLM PR before merging this one, usually having the implementation is a good way to check that the API is the right one.

@gragusa
Copy link
Author

gragusa commented Sep 10, 2022

I think it would be helpful to think about the API for dealing with rank-deficient models. For instance,
‘modelmatrix’ returns all the columns even those related to coefficients that cannot be estimated. I think this is fine as a default, but it would be useful to have a keyword argument to return only the columns corresponding to the estimable coefficients and a mechanism to identify the indexes of these columns. I have hacked these second part in this PR.

src/regressionmodel.jl Outdated Show resolved Hide resolved
residuals(model::RegressionModel; weighted::Bool=false)

Return the residuals of the model or, if `weighted=true`, the residuals multiplied by
the square root of the [model weights](@ref weights(::StatisticalModel)).
Copy link
Member

Choose a reason for hiding this comment

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

Where does the square root come from exactly? Doesn't that assume a particular definition of residuals (i.e. using L2-norm rather than e.g. L1-norm)?

Copy link
Author

Choose a reason for hiding this comment

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

Well, this is tricky. Also modelmatrix multiplies the entries of X by the square-root of the weights. Why?

Think about the linear model. With weights, the crossmodel matrix is $X'WX$. Then, to obtain it we can now do modelmatrix(lm1; weighted = true)'modelmatrix(lm1; weighted = true).

Notice that this is consistent with R; see, e.g., the function weighted.residuals which is in stats.

With weights, any weights is
$$\sqrt{w_i}y_i = \sqrt{w_i}x_i \beta + \sqrt{w_i}u_i.$$ So the understanding is that weighting single constituents of the model (y,x,u) amount to weight by $\sqrt{w_i}$.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, another tricky point. My understanding is that for residuals, the square root comes from the fact that deviance residuals themselves are defined as the square root of quantities which are partitions of the deviance. Right?

Note the R docstring for weighted.residuals says "Weighted residuals are based on the deviance residuals", which are only one kind of residual. Actually in R residuals also returns weighted residuals, except for response residuals, which are always unweighted. Maybe to be completely accurate we could say "for deviance and Pearson residuals...", so that packages are free to use different definitions (or throw an error) if needed?

Copy link

Choose a reason for hiding this comment

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

I think what @nalimilan says is that the assumption here (and in your change of modelmatrix) is that for all kinds of weights the weighted model matrix is X * sqrt.(W). Is it always true for FrequencyWeights, AnalyticWeights and ProbabilityWeights? x-ref: JuliaStats/GLM.jl#487

Copy link
Author

Choose a reason for hiding this comment

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

@bkamins weighted residuals, weighted model matrix do not exist in statistics. They are only useful from a coding point of view - they make it easier to write neater code.

I have always defined these quantities as multiplied by $\sqrt{w_i}$ as it is much more convenient. Some thing for R — which returns silently squared-root weighted residuals. Also other packages, notable FixedEffectModels.jl does that.

@nalimilan make sense what you propose - I will add more context to the doc

Copy link
Member

Choose a reason for hiding this comment

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

Even if these don't exist in statistics, the question can be phrased as "are there situations where the returned value is useful, even when you don't know the kind of weights used". I think the answer is yes, but it's tricky, so... R base only supports analytic weights so it's not a great reference.

src/statisticalmodel.jl Outdated Show resolved Hide resolved
gragusa and others added 2 commits October 19, 2022 11:16
Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>
Co-authored-by: Milan Bouchet-Valat <nalimilan@club.fr>
@gragusa gragusa changed the title Add keyword arg to modelmatrix; define momentfunction Add keyword arg to modelmatrix; define momentmatrix Nov 17, 2022
@gragusa
Copy link
Author

gragusa commented Nov 17, 2022

Something that would be very useful and that I would like to add o this PR is invloglikhessian - inverse of the hessian of the likelihood. The name is not great, but invloglikelihoodhessian is a monster, and I don't like invloglikhess (but I would do anything to have this merged.

With this method defined (whose implementation for GLM is part of JuliaStats/GLM.jl#487) CovarianceMatrices.jl could drop the GLM dependency, and implementation of all the covariances could be done via the API.

@nalimilan
Copy link
Member

Sure. Would it make sense to call it invhessian? Do other modeling packages define this function currently?

@gragusa
Copy link
Author

gragusa commented Dec 20, 2022

Sure. Would it make sense to call it invhessian? Do other modeling packages define this function currently?

I don't think so. The R package dealing with robust variances uses meat and bread which is not very clear (but in that case does not matter since R is not composable and methods can be extended only by the package itself).

We already have the inverse of the log hessian of the likelihood in glm -- is invchol. For linear models, the inverse of the normal log-likelihood is (X'X)^{-1}. But for general likelihoods this is not the case.

Now, invhessian is a little too generic, but I could live with it.

@nalimilan
Copy link
Member

I don't have a strong preference, but at least for consistency I think we should spell "likelihood" in full if we use that term. Luckily autocompletion will almost always work. ;-)

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