coefnames returns symbols #169
Conversation
|
Thanks for this! I don't think I'll have time to review this until mid Feb so ping me if I haven't weighed in by then.
… On Jan 20, 2020, at 11:35, Zachary P Christensen ***@***.***> wrote:
This PR begins to address #113. Right now it errors on the statsmodels tests. It's due to a problem with CoefTable. I think I would need to make a PR to StatsBase giving NoQuote the ability to handle Symbol. Other than that it seems to pass all tests.
So if you like this PR I'll take the next step and see if I can fix the problem in StatsBase.
You can view, comment on, or merge this pull request online at:
#169
Commit Summary
coefnames returns symbols
File Changes
M src/temporal_terms.jl (7)
M src/terms.jl (46)
M test/contrasts.jl (18)
M test/modelmatrix.jl (54)
M test/statsmodel.jl (14)
M test/temporal_terms.jl (10)
Patch Links:
https://github.com/JuliaStats/StatsModels.jl/pull/169.patch
https://github.com/JuliaStats/StatsModels.jl/pull/169.diff
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
This looks nice |
There was a problem hiding this comment.
This generally seems fine by me. I'm honestly not quite sure what the knock-on effects of this would be; at the very least packages like MixedModels that use this API would have to be updated, and maybe others. So it would be pretty disruptive but maybe worth it. I think a little more justification for why this is necessary is needed before we merge it though, as well as a bit of clean up.
| coefnames(ll.term) .* "_$opname$(ll.nsteps)" | ||
| end | ||
| StatsBase.coefnames(ll::LeadLagTerm{<:Any, F}) where F = _llcoef(ll, coefnames(ll.term), string(nameof(F.instance))) | ||
| _llcoef(ll::LeadLagTerm, t::Symbol, opname) = Symbol(t, "_$opname$(ll.nsteps)") |
There was a problem hiding this comment.
why can't you use the original broadcast based solution?
| @@ -543,18 +543,48 @@ vectorize(x) = [x] | |||
| coefnames(term::AbstractTerm) | |||
|
|
|||
| Return the name(s) of column(s) generated by a term. Return value is either a | |||
| `String` or an iterable of `String`s. | |||
| `Symbol` or an iterable of `String`s. | |||
| ["$(t.sym): $name" for name in t.contrasts.termnames] | ||
| StatsBase.coefnames(t::FunctionTerm) = string(t.exorig) | ||
| StatsBase.coefnames(ts::TupleTerm) = reduce(vcat, coefnames.(ts)) | ||
| StatsBase.coefnames(::InterceptTerm{H}) where {H} = H ? Symbol(:Intercept) : [] # this seems like the wrong thing to return |
There was a problem hiding this comment.
Can we keep the parens? Then you'd have Symbol("(Intercept)").
| """ | ||
| coef(term::AbstractTerm, s::Symbol) | ||
| """ | ||
| function StatsBase.coef(f::FormulaTerm, s::Symbol) |
There was a problem hiding this comment.
I don't see what the point of this is, or rather why it's called coef (which means something different in statsbase). can you explain what your goal is here? in any case, this needs tests if it's going to be included, and it seems like it would be better in a separate PR.
also, it doesn't seem like this works for categorical terms or for interactions, even if you specify the full coefficient name, since coefnames will be a vector of symbols.
and the RHS can have other things than just a MatrixTerm (e.g., MixedModels has a tuple of fixed and random effect matrices which are of different types)
|
One nice consequence of this would be is that it would be easier to create named tuples from various kinds of model outputs (I just found myself converting coefnames into Symbols to do this). |
|
Let's discuss whether it's a good idea or not at #113. |
|
Thanks for taking a look at this. I'll address the issues if people agree it's a good idea. It was admittedly a messy first attempt to see how practical implementing it would be |
This PR begins to address #113. Right now it errors on the
statsmodelstests. It's due to a problem withCoefTable. I think I would need to make a PR to StatsBase givingNoQuotethe ability to handleSymbol. Other than that it seems to pass all tests.So if you like this PR I'll take the next step and see if I can fix the problem in StatsBase.