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

Small Optimization of HC1 VCOV Computation #8

Merged
merged 10 commits into from
Aug 8, 2024

Conversation

s3alfisc
Copy link
Member

@s3alfisc s3alfisc commented Aug 7, 2024

Avoids creating a (potentially) big diagonal matrix.

Equivalence demonstration and benchmarks:

import pyfixest as pf
import numpy as np
import time

data = pf.get_data(N = 10_000)

fit = pf.feols("Y ~ C(f1) + C(f2)", data=data)
X = fit._X
uhat = fit._u_hat.reshape(-1,1)
print(np.allclose(X.T @ np.diag(uhat.flatten()) @ X, (X * uhat).T @ X))
# True

tic = time.time()
X.T @ np.diag(uhat.flatten()) @ X
toc = time.time()
print("first run", toc - tic)
# 0.517794132232666


tic = time.time()
(X * uhat).T @ X
toc = time.time()
print("second run", toc - tic)
# 0.010312080383300781

Will not matter for this package for 99% of use cases, but help with the occasional 1% =)

@s3alfisc s3alfisc changed the title Small Optimization of HC1 Meat Matrix Computation Small Optimization of HC1 VCOV Computation Aug 7, 2024
@apoorvalal apoorvalal merged commit 20ab6bf into py-econometrics:master Aug 8, 2024
4 checks passed
apoorvalal added a commit that referenced this pull request Aug 8, 2024
apoorvalal added a commit that referenced this pull request Aug 8, 2024
@apoorvalal
Copy link
Member

this broke SE estimation on my end btw (i initially merged because pr checkout didn't work and but then reverted); i'll take a look at what might be going on later.

@apoorvalal
Copy link
Member

this prompted me to add an SE test; maybe rebase on main and test against that

@s3alfisc
Copy link
Member Author

s3alfisc commented Aug 8, 2024

Oh wow, sorry about that! Should have tested more thoroughly. Will rebase and test against the new set of tests. Thanks!

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