Add context capture to formula materialisation

[adamvig96]

Context: #769

[s3alfisc]

pre-commit.ci autofix

[adamvig96]

@s3alfisc I added a unit test.

Would you like to add some integration tests too? if so, I'd be happy to have some guidance on where to put those. Thanks!

[s3alfisc]

Hi @adamvig96 , looking very good so far, thank you! Could you add integration into test_api.py? I guess that testing equality of coefficients and SEs of creating splines via the new capture arg + formulaic vs "by hand" should do the job?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Flag	Coverage Δ
core-tests	82.41% <100.00%> (+0.31%)	⬆️

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

Files with missing lines	Coverage Δ
pyfixest/estimation/FixestMulti_.py	80.59% <100.00%> (+0.39%)	⬆️
pyfixest/estimation/estimation.py	97.08% <100.00%> (+0.11%)	⬆️
pyfixest/estimation/feiv_.py	98.21% <100.00%> (+0.01%)	⬆️
pyfixest/estimation/feols_.py	83.93% <100.00%> (+0.03%)	⬆️
pyfixest/estimation/feols_compressed_.py	69.59% <100.00%> (+0.17%)	⬆️
pyfixest/estimation/fepois_.py	87.38% <100.00%> (+0.05%)	⬆️
pyfixest/estimation/model_matrix_fixest_.py	85.32% <100.00%> (+0.24%)	⬆️
pyfixest/utils/utils.py	92.30% <100.00%> (+0.30%)	⬆️

... and 1 file with indirect coverage changes

[adamvig96]

pre-commit.ci autofix

[adamvig96]

Hi @s3alfisc! I added an integration test, wdyt is this enough? Thanks! :)

[s3alfisc]

Yes, I think so, thank you! Will review and merge later today. One thing that wasn't clear to me - why is the argument set to 0 be default and set to 2 in the code if no mapping is provided?

[adamvig96]

I set it to 0 because it was the default in formulaic, but formulaic always adds one when calling their capture_context function (see this for example). I wanted to avoid that repetition of checking whether we got an int (and the add +1) or a mapping, that's why I created the wrapper function in utils.

We should probably set it to 1 by default and adding +1 in our wrapper function, maybe that's more readable?

[s3alfisc]

Hi @adamvig96 , sorry for the slight delay - I took a look at the formulaic code base and it looks like the get_model_matrix method expects a mapping or None: only what is passed as context to the method will be available for creating the design matrix. So I think that we could drop the context() function and explicitly pass the context argument to the method?

https://github.com/matthewwardrop/formulaic/blob/ddb3c11dbb8cb661234142a753d7bea1f7a644b5/formulaic/model_spec.py#L684

[s3alfisc]

pre-commit.ci autofix

[adamvig96]

Hi @s3alfisc, thanks for taking a look! If I understand your proposal correctly, that would mean that we only call the capture_context function at the feols/fepois api level – we need to call the function somewhere to get the default context, i think – and from there on, the context is just passed through until it reaches formulaic's get_model_matrix?

[s3alfisc]

I actually just realized that I misunderstood how everything works, and what I suggested above doesn't make sense / work. Sorry!

[s3alfisc]

By setting the defaults to context = 0, we basically always pass the entire context of the feols call to get_model_matrix, right? Do you think there is any downside to that, i.e. could there be side effects? For example, could it happen that users accidentally pass something that overwrites internal behavior? (Currently I don't think so). Maybe it would be safer to set the default to None and to explicitly ask users to either provide the context as a mapping, or to use the "global" context? Something along these lines:

def feols(context=None):
    if context is None:
        context = {}
    elif isinstance(context, int):
        context = capture_context(context)

[adamvig96]

pre-commit.ci autofix

[adamvig96]

Hi @s3alfisc, I made the context parameter optional in feols and fepois.

[s3alfisc]

pre-commit.ci autofix

[s3alfisc]

pre-commit.ci autofix

[s3alfisc]

@all-contributors please add @adamvig96 for code

[allcontributors]

@s3alfisc

I've put up a pull request to add @adamvig96! 🎉

[s3alfisc]

pre-commit.ci autofix

[s3alfisc]

pre-commit.ci autofix

[s3alfisc]

pre-commit.ci autofix

[s3alfisc]

Sorry for all the noise @adamvig96 , I promise I will not code in the github browser going forward ... 🤕

[s3alfisc]

And merged! Thank you Adam @adamvig96 =)

[adamvig96]

@s3alfisc, thanks for the support! 💯