py-econometrics / pyfixest

Fast High-Dimensional Fixed Effects Regression in Python following fixest-syntax
https://py-econometrics.github.io/pyfixest/
MIT License
175 stars 35 forks source link

Adding literals to feols and fepois api's #680

Closed marcandre259 closed 2 weeks ago

marcandre259 commented 3 weeks ago

Reference to #671. Pretty literal interpretation. Added a small validate_literal_argument function, taken from the predict method of Feols (assumption is it may be used elsewhere also).

marcandre259 commented 3 weeks ago

pre-commit.ci autofix

codecov[bot] commented 3 weeks ago

Codecov Report

Attention: Patch coverage is 90.47619% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
pyfixest/estimation/literals.py 83.33% 2 Missing :warning:
Flag Coverage Δ
core-tests 77.73% <90.47%> (-0.06%) :arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
pyfixest/did/estimation.py 96.55% <100.00%> (+0.06%) :arrow_up:
pyfixest/did/lpdid.py 94.62% <100.00%> (+0.05%) :arrow_up:
pyfixest/estimation/__init__.py 100.00% <100.00%> (ø)
pyfixest/estimation/estimation.py 95.91% <100.00%> (+0.04%) :arrow_up:
pyfixest/estimation/feols_.py 83.06% <100.00%> (+0.07%) :arrow_up:
pyfixest/estimation/feols_compressed_.py 83.43% <100.00%> (ø)
pyfixest/estimation/fepois_.py 87.20% <100.00%> (-1.90%) :arrow_down:
pyfixest/estimation/literals.py 83.33% <83.33%> (ø)

... and 1 file with indirect coverage changes

marcandre259 commented 3 weeks ago

pre-commit.ci autofix

s3alfisc commented 3 weeks ago

pre-commit.ci autofix

s3alfisc commented 3 weeks ago

It's a little bit annoying, but whenever the autofix corrects code, one has to run it again to pass the checks (because the previous did not pass as it triggered the fix) @marcandre259

s3alfisc commented 3 weeks ago

Looks like mypy is complaining about the use of the _LiteralGenericAlias:

pyfixest/estimation/literals.py:1: error: Module "typing" has no attribute "_LiteralGenericAlias"  [attr-defined]

Maybe we can simply check for a literal type via smth like

    if get_origin(literal) is not Literal:
        raise TypeError("Expected a Literal type as the second argument")

?

I would not overthink this as it's in the end behavior we would (hopefully) catch in a review process?

s3alfisc commented 3 weeks ago

Another thing I oversaw in my initial review - feols and fepois check input types in https://github.com/py-econometrics/pyfixest/blob/a3ed27aefef01e80ade7ebf117d5999239bbb62b/pyfixest/estimation/estimation.py#L617

Maybe we could update them to use the Literal validation function whenever relevant?

marcandre259 commented 3 weeks ago

Looks like mypy is complaining about the use of the _LiteralGenericAlias:

pyfixest/estimation/literals.py:1: error: Module "typing" has no attribute "_LiteralGenericAlias"  [attr-defined]

Maybe we can simply check for a literal type via smth like

    if get_origin(literal) is not Literal:
        raise TypeError("Expected a Literal type as the second argument")

?

I would not overthink this as it's in the end behavior we would (hopefully) catch in a review process?

Hi @s3alfisc,

Indeed, I'll modify it with a similar check. I did notice that Literal was not really a type without being specific Literal["..."], at least when trying out type(literal) is Literal. But maybe it works with get_origin then?

marcandre259 commented 3 weeks ago

pre-commit.ci autofix

s3alfisc commented 2 weeks ago

Thanks @marcandre259 ! This looks good to me know =)

s3alfisc commented 2 weeks ago

Just fyi @marcandre259 , the test failures in the main branch stem back from changes to the actions wfl I merged yesterday and are completely unrelated to this PR =)