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

Make coeftable objects conform to Tables API #527

Closed
pdeffebach opened this issue Sep 25, 2019 · 12 comments
Closed

Make coeftable objects conform to Tables API #527

pdeffebach opened this issue Sep 25, 2019 · 12 comments

Comments

@pdeffebach
Copy link

If all coeftable objects returned by statistical packages are, indeed, tables. Then it seems reasonable to mandate that they conform to the Tables.jl API. This would allow seamless inter-op with good printing methods like PrettyTables.jl and some latex printing packages that work with DataFrames.

Of course, this would require going to lots of different packages and adding PRs for them to conform to this new guideline.

@nalimilan
Copy link
Member

Sure. The main problem is whether it's OK to add a dependency on Tables.jl. If we moved all modeling features to StatsModels, that wouldn't be a problem anymore.

Of course, this would require going to lots of different packages and adding PRs for them to conform to this new guideline.

CoefTable lives in StatsBase, so I don't think other packages need to be adapted.

@pdeffebach
Copy link
Author

Oh I thought that modeling packages were required to include the function coeftable but the implementation was up to them beyond that. Glad to hear it is standardized in StatsBase.

I support adding a dependency on Tables.jl. This seems to me to be exactly the kind of scenario it was created for.

@pdeffebach
Copy link
Author

Thinking about this more there might be more (breaking) quality of life improvements that would be nice. Being able to index coef(model) by term, or have confint(model) return a vector of tuples rather than a matrix.

@nalimilan
Copy link
Member

Yes it's too bad that these return plain vectors without names. Unfortunately, I'm not sure we can depend on NamedArrays or AxisArrays. Probably something to reconsider when we move these features to StatsModels.

@Nosferican
Copy link
Contributor

I don't think we would need to add Tables.jl as a dependency. We can just have the basic row-base Table-like and people can just call the Tables.API for it. That might also help to make it easier to format (e.g., to TeX). Related Nosferican/Econometrics.jl#21.

As for coef and confint to take an argument for specifying the terms I think that is quite reasonable. Might be implemented in Econometrics.jl if anyone opens an issue for it.

@nalimilan
Copy link
Member

I don't think we would need to add Tables.jl as a dependency. We can just have the basic row-base Table-like and people can just call the Tables.API for it. That might also help to make it easier to format (e.g., to TeX). Related Nosferican/Econometrics.jl#21.

I guess we could make CoefTable <: AbstractVector{<:NamedTuple}. Though it would be more logical and efficient to implement the column-wise interface, which AFAICT isn't possible without overloading Tables.jl functions. Cc: @quinnj

As for coef and confint to take an argument for specifying the terms I think that is quite reasonable. Might be implemented in Econometrics.jl if anyone opens an issue for it.

Yes, passing arguments would work. StatsModels could provide the necessary helpers so that it works for any model, by extracting the corresponding values from the vector returned by coef. Though that's a bit different from being able index directly the vector itself.

@Nosferican
Copy link
Contributor

I believe (parameter = [...], estimate = [...], stderror = [...]) is the native columntable like equivalent.

@nalimilan
Copy link
Member

Yes but that doesn't have pretty printing.

@Nosferican
Copy link
Contributor

We could try using CoefTable as a wrapper for a nice show method, but keep the accessors and make it <:AbstractVector{<:NamedTuple} so it works with the Tables API.

@nalimilan
Copy link
Member

Yes that's what I said.

@quinnj
Copy link
Member

quinnj commented Oct 14, 2019

Though it would be more logical and efficient to implement the column-wise interface, which AFAICT isn't possible without overloading Tables.jl functions.

Correct; to hook into Tables.columns, you have to explicitly overload Tables.columnaccess and Tables.columns. For Tables.rows, if your object is iterable, we'll try to treat it as a row-iterator, but it would be pretty hard to do the same in the column case.

@nalimilan
Copy link
Member

Fixed by #629.

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

No branches or pull requests

4 participants