Open fnaeher opened 2 years ago
I don't know this model, so I can't tell if it actually makes sense to add support for this. I am just adding a replicable example for convenience, not as a comment on the feature request:
library(fixest)
data(base_stagg)
sunab.agg = feols(y ~ x1 + sunab(year_treated, year) | id + year, base_stagg)
predict(sunab.agg, newdata = base_stagg)
#> Error in mm_info_new[[i]]: subscript out of bounds
The philosophy of the software is: either it works, either it's an exception properly handled. So that's definitely a bug! 😄
Thanks for reporting!
Edit: Sorry, ignore my comment. It’s not a dup.
The problem is that the "model matrix info" for newdata
is not the same as it is in the original model.
library(fixest)
base_stagg <- fixest::base_stagg
sunab.agg <- feols(y ~ x1 + sunab(year_treated, year) | id + year, base_stagg)
mm_info <- sunab.agg$model_matrix_info
rhs_fml <- fixest:::fml_split(sunab.agg$fml, 1)
mm_new <- fixest:::fixest_model_matrix_extra(
object=sunab.agg, newdata=base_stagg, original_data=FALSE, fml=rhs_fml,
i_noref=TRUE
)
mm_info_new = attr(mm_new, "model_matrix_info")
In predict.fixest
there is a comment stating:
As you can see below, this is not the case:
str(mm_info)
#> List of 2
#> $ :List of 8
#> ..$ coef_names_full: chr [1:18] "year::-9" "year::-8" "year::-7" "year::-6" ...
#> ..$ items : num [1:18] -9 -8 -7 -6 -5 -4 -3 -2 -1 0 ...
#> ..$ ref_id : int 9
#> ..$ ref : num -1
#> ..$ f_name : chr "year"
#> ..$ is_num : logi TRUE
#> ..$ is_inter_fact : logi FALSE
#> ..$ is_inter_num : logi FALSE
#> $ sunab:List of 4
#> ..$ agg : chr "(\\Qyear\\E)::(-?[[:digit:]]+):cohort"
#> .. ..- attr(*, "model_matrix_info")=List of 8
#> .. .. ..$ coef_names_full: chr [1:18] "year::-9" "year::-8" "year::-7" "year::-6" ...
#> .. .. ..$ items : num [1:18] -9 -8 -7 -6 -5 -4 -3 -2 -1 0 ...
#> .. .. ..$ ref_id : int 9
#> .. .. ..$ ref : num -1
#> .. .. ..$ f_name : chr "year"
#> .. .. ..$ is_num : logi TRUE
#> .. .. ..$ is_inter_fact : logi FALSE
#> .. .. ..$ is_inter_num : logi FALSE
#> ..$ agg_att : Named chr "\\Qyear\\E::[[:digit:]]+:cohort"
#> .. ..- attr(*, "names")= chr "ATT"
#> ..$ agg_period: chr "(\\Qyear\\E)::(-?[[:digit:]]+):cohort"
#> ..$ ref.p : num -1
str(mm_info_new)
#> List of 1
#> $ sunab:List of 4
#> ..$ agg : chr "(\\Qyear\\E)::(-?[[:digit:]]+):cohort"
#> .. ..- attr(*, "model_matrix_info")=List of 8
#> .. .. ..$ coef_names_full: chr [1:18] "year::-9" "year::-8" "year::-7" "year::-6" ...
#> .. .. ..$ items : num [1:18] -9 -8 -7 -6 -5 -4 -3 -2 -1 0 ...
#> .. .. ..$ ref_id : int 9
#> .. .. ..$ ref : num -1
#> .. .. ..$ f_name : chr "year"
#> .. .. ..$ is_num : logi TRUE
#> .. .. ..$ is_inter_fact : logi FALSE
#> .. .. ..$ is_inter_num : logi FALSE
#> ..$ agg_att : Named chr "\\Qyear\\E::[[:digit:]]+:cohort"
#> .. ..- attr(*, "names")= chr "ATT"
#> ..$ agg_period: chr "(\\Qyear\\E)::(-?[[:digit:]]+):cohort"
#> ..$ ref.p : num -1
For most applications,
fixest
andmarginaleffects
packages interact well. However, it looks as iffixest
does not support the estimation of marginal aggregated cohort effects and att as according to Sun & Abraham (2020) withmarginaleffects
. Please also see the short discussion at https://github.com/vincentarelbundock/marginaleffects/issues/315#issue-1239702882. It is suggested thatfixest
does not supply apredict
method for aggregated cohort and att estimates that accepts anewdata
argument which can be passed on tomarginaleffects
. If this is the case indeed, I would suggest to add this functionality tofixest
. Thank you very much!