Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add drop_singletons option to partial_out() #263

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

droodman
Copy link
Contributor

@droodman droodman commented Apr 3, 2024

Before handing things over to ivreg2, ivregdfe partials out the FE and identifies and drops singletons. I'm making reghdfejl optionally do the same thing now--call partial_out() and then use ivreg2 instead of FixedEffectsModels.jl for the core IV work--since it offers a lot more diagnostics and estimation options. So it would be good if partial_out() also optionally dropped singletons. I made the new drop_singletons option default to false so old results are not affected. Of course, this is inconsistent with the default for drop_singletons being true in fit().

Copy link

codecov bot commented Apr 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.49%. Comparing base (72ca0e0) to head (23688f0).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #263      +/-   ##
==========================================
+ Coverage   96.48%   96.49%   +0.01%     
==========================================
  Files           8        8              
  Lines         655      657       +2     
==========================================
+ Hits          632      634       +2     
  Misses         23       23              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@matthieugomez
Copy link
Member

Thanks. the contract of `partial_out' is to return a dataframe with as many rows as the initial one, as long as align = true.
Could you add a check that, when drop_singletons = true is specified, the function returns an error unless align is set to false? Thanks

@matthieugomez
Copy link
Member

matthieugomez commented Apr 8, 2024

Another thing is that adding a fourth argument to the function return is breaking. I don't mind, i could tag a new version, but at this point, it'd be better to create a return type for partial_out.

@@ -135,6 +138,8 @@ function partial_out(
residuals .= residuals .+ m
end

dof_fes = mapreduce(nunique, +, fes, init=0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would find sum(nunique(fe) for fes) easier to read

@droodman
Copy link
Contributor Author

I understand you want to be careful in changing the interface of partial_out() in a breaking way. But I'm not sure what "contract" means here. This package is not fully documented; in particular, partial_out() is not documented. And in general I think a mature API will have a functional interface so the caller need not interact directly with, and make assumptions about, the structure of the return object. E.g., as far as I know the StatsAPI() interface is all functional.

So I would propose adding new return values onto the end of the currently returned tuple, to be minimally breaking; documenting this function an and any others that are conceived of as making contractual commitments; and implementing the commitments through functions.

@matthieugomez
Copy link
Member

partial_out is documented — have a look at all the documentation info at the top of the file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants