Fixes #933. @s3alfisc updated to new PR without .idea/ or the entire history needing to merge again.

Codecov Report

Attention: Patch coverage is 66.66667% with 5 lines in your changes missing coverage. Please review.

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

... and 5 files with indirect coverage changes

Hm for some reason the JAX demeaner does not seem to fail with fixef_maxiter = 1:

    def test_demean_model_maxiter_convergence_failure(demean_func):
         """Test that demean_model fails when maxiter is too small."""
         N = 100
         rng = np.random.default_rng(42)
    
         Y = pd.DataFrame({"y": rng.normal(0, 1, N)})
         X = pd.DataFrame({"x1": rng.normal(0, 1, N)})
         # Many fixed effects to make convergence difficult
         fe = pd.DataFrame({"fe1": np.arange(N)})  # Each obs is its own FE
         weights = np.ones(N)
         lookup_dict = {}
    
         # Should fail with very small maxiter
>        with pytest.raises(ValueError, match="Demeaning failed after 1 iterations"):
E        Failed: DID NOT RAISE <class 'ValueError'>

@all-contributors please add @damandhaliwal for code

@s3alfisc

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

Made a small update to the tests - with only one fixed effect, there is not much to iterate, we simply demean x and y, it's one-shot. I think with two fixed effects, this should be different.

pre-commit.ci autofix

Tried quite a bit but will not manage for tonight. Seems like a more subtle issue that you have uncovered ... Sorry about that, I really thought this would be more of a "quick win".

I think all the tests ran before I pushed yesterday. I'll also take another look tonight and see if I can figure it out. No worries on the "easy" win, would rather have it be tough and get me to figure stuff out

I think all the tests ran before I pushed yesterday. I'll also take another look tonight and see if I can figure it out. No worries on the "easy" win, would rather have it be tough and get me to figure stuff out

Ah really? Maybe you did not run tests for the JAX and Rust backends? 🤔

Btw, I just had made my mind I though "let's merge with the error as it's unrelated and fix it in a separate PR" but now you've got me thinking again. Tbh I would be surprised if everything ran without error in the other branch. Unfortunately gh already deleted all the CI logs.

No worries on the "easy" win

I love easy wins 😄

Btw, I just had made my mind I though "let's merge with the error as it's unrelated and fix it in a separate PR" but now you've got me thinking again. Tbh I would be surprised if everything ran without error in the other branch.

Okay, I was mistaken. I ran it again and the test does fail for demean_jax. I can look into it tomorrow, probably need to run some examples to find where it goes a different way. My guess is that it doesn't converge either, but just does not raise the same error due to some difference

@s3alfisc I fixed it. Wasn't an underlying big issue in estimation. The return variable converged in demean_jax_.py was of the format jax.array but the test was expecting just a boolean so changed the return to bool(converged). Let me know what you think. Tests passed locally.

Ah awesome! Thank you @damandhaliwal. I'll do a final review then and merge.

pre-commit.ci autofix

The linter still fails but this is unrelated to your PR, I will fix this in a separate PR =)