Added solver to feols and created new test file for it

[saidamir]

Hi I added a solver to feols class and created a test file for it. Once approved, I will add the solver to other classes as well, as I want to complete one class at this stage.

[s3alfisc]

Looks good! For other solvers, maybe we can add a dedicated function, e.g.

def _solve_ols(tZX, tZY, solver):
   
    if solver== "np.linalg.lstsq":
        return np.linalg.lstsq(tZX, tZy, rcond=None)[0]
    elif solver == "np.linalg.solve": 
        return np.linalg.solve(tZX, tZy).flatten()
    else: 
        raise ValueError(f"Solver {solver} not supported.")
        

[saidamir]

You want me to do it for other classes, and leave this calls feols as it is now?

[saidamir]

@s3alfisc would you please review my pr at your free time? Thanks!

[s3alfisc]

Hi @saidamir, sorry, I did not understand that this was already ready for review. This looks good, thank you! =) What next step do you have in mind? Will you add solver arguments to Fepois and Feiv or add arguments for the feols() and fepois() APIs?

[saidamir]

The test is failing per logs, as I can see, and the error:
"FAILED tests/test_solver.py::test_solver_equivalence - TypeError: Y must be a numpy array."
I could not figure out why this was happening, it looks to me that this bug is not cause by my changes but it was in the original design of the get_data() method and feols class. The output of get_data() is not compatible with Feols methods. What do you think?

[s3alfisc]

Hi Aziz (@saidamir) - if you move the solve_ols method to Feols and delete it in the Feiv and Fepois classes, then this looks good to merge (pending CI tests not failing, which I don't expect).

@all-contributors please add @saidamir for code.

[allcontributors]

@s3alfisc

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

[saidamir]

Agreed.

[saidamir]

You want me to do it for other classes, and leave this calls feols as it is now?

[saidamir]

agreed

[saidamir]

agreed

[saidamir]

@s3alfisc When running test_solver.py the following error:

FAILED tests/test_solver.py::test_solver_fepois - ValueError: operands could not be broadcast together with shapes (997,997) (997,998).

The proposed fix would involve changes to fepois_.py, git_fit() method as follows: Ensure mu is reshaped to (n_samples, 1) for correct broadcasting:

- WX = np.sqrt(mu) * X_resid
+ WX = np.sqrt(mu.reshape(-1, 1)) * X_resid

I am not sure if I can change the git fit method. What do you think?

[s3alfisc]

Nice catch! I think the error stems from

delta_new = self.solve_ols(XWX, XWZ, _solver) 

now evaluating to a "flat" array whereas in the old version, it was of dim N x 1. In the second run of the for loop, we then get the shape error you report.

I think changing delta_new back to a N x 1 array should do the trick, e.g.

delta_new = self.solve_ols(XWX, XWZ, _solver).reshape((-1,1))

[s3alfisc]

I think everything should be fixed now. I also added support for scipy.sparse.linalg.lsqr and added a test =)

[codecov]

Codecov Report

Attention: Patch coverage is 98.63014% with 1 line in your changes missing coverage. Please review.

Files	Coverage Δ
pyfixest/estimation/feiv_.py	97.67% <100.00%> (ø)
pyfixest/estimation/fepois_.py	89.18% <100.00%> (ø)
tests/test_solver.py	100.00% <100.00%> (ø)
pyfixest/estimation/feols_.py	87.52% <91.66%> (ø)

... and 26 files with indirect coverage changes

[s3alfisc]

And it's merged! Congrats @saidamir and thanks for your help! =)

[saidamir]

@s3alfisc would you recommend me the next issue? I have to admit, still struggling a bit with the basics, as I have to combine work, jobs search and this very exciting project, but I love it!

[s3alfisc]

Of course! The immediate follow up would consist of adding the solver argument to the feols and fepois functions. There's no PR for this yet but I can write one tomorrow evening =)