Allow missings in the modelmatrix, necessary to use lead/lag in @formula

[greimel]

With lead/lag in a @formula the model matrix contains missings. I adjusted the code so that the output of modelmatrix is checked for missings and and observations with missings are removed.

MWE (can be added as test)

using DataFrames, FixedEffectModels

df = DataFrame(y = randn(10), x = randn(10))
f0 = @formula(y ~ x + lag(x, 1))
reg(df, f0) # errors without this PR

This becomes more useful when defining groupedlag for panel data, as I did here: JuliaStats/StatsModels.jl#182

This is an unsolved issue also for GLM.jl: JuliaStats/GLM.jl#366

[matthieugomez]

Arg, thanks for spotting the issue. I remember raising this issue a while ago (https://discourse.julialang.org/t/psa-breaking-changes-in-statsmodels-v0-6-0-terms-2-0-son-of-terms/25626/4?u=matthieu).
The one thing I do not like with your solution is that it requires to create temporary matrices, which can be very wasteful for big datasets. I think I would prefer to wait until there is a better way to handle this in StatsModels.

[matthieugomez]

In the meantime, it would be awesome if you could do a pull request that returns a better error explaining the problem.

[greimel]

EDIT: I see now, that my point was wrong. One can first filter by complete cases of weights and cluster and pass a view to apply_schema.

[greimel]

Wouldn't you think that this is a better temporary solution than a better error message?

It would be nice to be able to use lags with your package.

[matthieugomez]

I don't think it is worth it if it makes all regressions that do not use lag slower (or unfeasible). Moreover, lag does not even work with timed variable, or within groups (for now), so I'm not sure it's that useful for the kind of things people do with the package.

[greimel]

I added a commit so that regressions without lags should not be affected.

It's true that the lead and lag functions in StatsModels are not very powerful yet. I extended the lag function for groups in JuliaStats/StatsModels.jl#182, where it was suggested to put it in a separate package for now: https://github.com/greimel/GroupedTemporalTerms.jl

Does this address all of your concerns, or are there more?

[matthieugomez]

I think it would be better to do

for c in eachcol(X)
          esample2 .&= .!ismissing.(c)
 end

[greimel]

addressed in new commit

[matthieugomez]

could you add a comment to the issue number

[greimel]

addressed in new commit, please let me know if the comments are overkill now

[matthieugomez]

If you want it merged, could you answer the comments and add tests?

[greimel]

This version passes a test comparing hand-lagged to auto-lagged regression (with fixed effects, but no weights, no clustering, no iv) on private data.

I will push a comprehensive test soon.

Would you accept a test dependency on https://github.com/greimel/GroupedTemporalTerms, or should I use non-grouped lags?

[greimel]

I added tests, tests pass locally

This is ready for review. If desired I can force push three logical commits. I needed this for debugging.

[greimel]

I realized that I missed one case. The case where the df contains missings and the formula produces missings cannot be handled yet. This because the missings in the df are removed before the the lag is applied.

I made the reg function throw an ArgumentError in that case.

[greimel]

case where the df contains missings and the formula produces missings cannot be handled yet

I actually need this case for my research. So I am gonna try to handle this case as well. You might want to wait with merging this until then if you wish.

I am about to push some commits that make the analogous changes to the partial_out.

[greimel]

Thanks!

[eloualiche]

Reverted this for the time being out of concern with memory allocation. Sorry for prematurely approving the PR.