search

FixedEffects / FixedEffectModels.jl

Fast Estimation of Linear Models with IV and High Dimensional Categorical Variables
Other
225 stars 46 forks source link

Bring `predict` back #245

Closed nilshg closed 10 months ago

nilshg commented 11 months ago

This PR takes another shot at a predict method for fixed effect models, with the aim of closing #204 and #243.

While this implementation solves the issues with missing data raised in #204, it does not fix the fe(x1)&x2 interaction case. Instead I've written a rather hacky solution that attempts to detect these cases and error out.

The issue I ran into is that once an fe term is involved, we can't rely on the StatsModels magic to create our X matrix for us and then multiply by coefficients. While it wouldn't be too hard to fix this for the simple fe(x1)&x2 case, I quickly realized that this doesn't then cover fe(x1)&fe(x2)&x3 and lots of other permutations one could think up. All in all I was going down a path of re-implementing the modelmatrix machinery which didn't seem right, so I stopped and copped out with the error.

Would be good to get opinions from @matthieugomez @eloualiche as to whether a band aid like this is acceptable to at least get the basic functionality in, maybe also @jariji who took an interest in #243 in having this. I would also be interested in improving this PR by adding confidence intervals if anyone's got ideas on how to do this.

codecov[bot] commented 11 months ago

Codecov Report

All modified lines are covered by tests :white_check_mark:

Comparison is base (6d23721) 95.78% compared to head (d0ad15b) 96.35%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #245 +/- ## ========================================== + Coverage 95.78% 96.35% +0.57% ========================================== Files 8 8 Lines 640 659 +19 ========================================== + Hits 613 635 +22 + Misses 27 24 -3 ``` | [Files](https://app.codecov.io/gh/FixedEffects/FixedEffectModels.jl/pull/245?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | Coverage Δ | | |---|---|---| | [src/FixedEffectModel.jl](https://app.codecov.io/gh/FixedEffects/FixedEffectModels.jl/pull/245?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-c3JjL0ZpeGVkRWZmZWN0TW9kZWwuamw=) | `95.20% <100.00%> (+1.96%)` | :arrow_up: | ... and [1 file with indirect coverage changes](https://app.codecov.io/gh/FixedEffects/FixedEffectModels.jl/pull/245/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

matthieugomez commented 10 months ago

Yes, I think a band aid like this is acceptable.