lrberge / fixest

Fixed-effects estimations
https://lrberge.github.io/fixest/
361 stars 59 forks source link

`l()` error with named parameter #499

Closed MaelAstruc closed 1 month ago

MaelAstruc commented 1 month ago

Hello,

I have encountered an issue with the feols() function when using l() with named parameters.

Here is a short example with a panel data from plm:

data("EmplUK", package="plm")

EmplUK <- fixest::panel(EmplUK, panel.id = ~firm+year)

fixest::feols(output ~ l(capital, 1L), EmplUK)

# NOTE: 140 observations removed because of NA values (RHS: 140).
# OLS estimation, Dep. Var.: output
# Observations: 891
# Standard-errors: Clustered (firm) 
# Estimate Std. Error    t value  Pr(>|t|)    
# (Intercept)   102.950589   0.326763 315.061991 < 2.2e-16 ***
#   l(capital, 1)  -0.023391   0.044998  -0.519814   0.60402    
# ---
#   Signif. codes:  0 '***' 0.001 '**' 0.01 '*' 0.05 '.' 0.1 ' ' 1
# RMSE: 9.56003   Adj. R2: -8.885e-4

fixest::feols(output ~ l(capital, lag = 1L), EmplUK)

# Erreur dans fixest::feols(output ~ l(capital, lag = 1L), EmplUK) : 
#  Problem in the formula regarding lag/leads: 
#  In l__expand(capital, lag = 1): argument inutilisé (lag = 1)

From my understanding, the error rises because l__expand() expects a parameter 'k' instead of 'lag' or 'lead'.

I think this could be fixed by:

lrberge commented 1 month ago

Assuming that nobody uses the named parameter and ignore this issue

Well, one shall never ignore a bug! :-)

I followed the variable renaming path (e989d67). It's the one that makes the most sense because it becomes in line with the lag.formula function. (Otherwise the argument names differed for the same functionality, which is bad.)

I also reworked the error message which shouldn't have shown l__expand which is internal cuisine.

Thanks for reporting!