Conversation
There was a problem hiding this comment.
Pull request overview
This PR optimizes Gaussian GLMs with an identity link by adding a direct (closed-form) weighted least squares solve, avoiding the IRLS-based iteration in that special case.
Changes:
- Add a closed-form linear-system solve for Normal + Identity-link models under the unconstrained, no-L1 path.
- Fall back to the existing IRLS path when the direct solve fails.
- Document the change in the changelog.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
src/glum/_glm.py |
Adds a closed-form Gaussian/identity solver and a fast-path branch in _solve. |
CHANGELOG.rst |
Notes the new closed-form solve behavior in the 3.1.3 unreleased section. |
Comments suppressed due to low confidence (1)
CHANGELOG.rst:23
- This release section now has two separate Other changes: headers (one at line 13 and another at line 21). Consider merging this new bullet into the existing Other changes: section to keep the changelog structure consistent and avoid duplicate headings.
**Other changes:**
- Use a closed-form solution for Gaussian (identity-link) models (ridge and OLS), with automatic fallback to the iterative solver for singular systems.
**Bug fix:**
- Fixed ``deviance_path_`` in :class:`~glum.GeneralizedLinearRegressorCV` being scaled down by a factor of ``n_folds`` because test fold weights were not normalized to sum to 1.
**Other changes:**
- We disabled fast math to avoid invalid results (e.g., when dividing by zero).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
MatthiasSchmidtblaicherQC
left a comment
There was a problem hiding this comment.
Nice work and good speedup! Just some comments on code structure.
stanmart
left a comment
There was a problem hiding this comment.
Looks great! I would have thought that using the IRSL solver with an OLS results in a single iteration, i.e., essentially a closed-form solution, but there is a significant speedup, which is awesome. I only have a couple of very minor comments.
Use closed-form solution for Gaussian (identity-link) models
This PR adds a closed-form solve for Gaussian GLMs with identity link (i.e. linear regression), replacing the IRLS-based iterative solve in this special case.
For Gaussian + identity link, the objective reduces to weighted least squares (with optional L2 penalty), which has the closed-form solution
(Xᵀ W X + P2) β = Xᵀ W y
We now solve this linear system directly and fall back to the existing IRLS solver if the system is singular or ill-conditioned.
This change improves performance for Gaussian (ridge and OLS) models without changing the public API or behavior.
Motivation
While updating the benchmark, I realized that sklearn outperforms glum on L2 Gaussian problems because sklearn uses a closed form solution in this case, whereas we perform one iteration.
As we constrained the solvers to one iteration in those cases, this is not a significant difference, and the solvers are already quite fast.
This is not a high priority for us, I just implemented it for the sake of completeness and to optimize Glum's performance in these cases.
Results:
I compared the runtime for all Gaussian-L2 problems, both with and without using the closed-form solution. These are the results:
As you can see, the closed-form solution is faster for every problem, while achieving the same results. On average, there is a 27.7% speed increase across all the problems. As previously mentioned, these runtimes are already low, so the absolute difference is minimal.
This screenshot shows the runtime and objective comparison between glum and sklearn for the problems for which the closed form solution will be applied. As you can see, Glum achieves the same results as Sklearn, but is even faster in some cases.
I did this for the sake of completeness and because I was personally interested in the impact. However, if you decide against integrating it, I have no problem with that, given that the absolute runtime saving is very low for these already fast cases.