Repository navigation
Include dropped first column in monotonic constraints - #1023
Closed
Matthias Schmidtblaicher (MatthiasSchmidtblaicherQC) wants to merge 6 commits into
Closed
Matthias Schmidtblaicher (MatthiasSchmidtblaicherQC) wants to merge 6 commits into
Matthias Schmidtblaicher (MatthiasSchmidtblaicherQC) wants to merge 6 commits into
Conversation
Matthias Schmidtblaicher (MatthiasSchmidtblaicherQC)
requested a review
from Alex Sauer (AlexanderSauerQC)
October 27, 2026 17:04
bs() without include_intercept and categoricals with drop_first lack their first column, whose coefficient is implicitly zero. Constrain the first remaining coefficient against it so the fitted effect is monotone. Also document monotonic_constraints in the estimator docstrings and plot the fitted effect in the formula tutorial. Closes #1021
Marc-Antoine Schmidt (MarcAntoineSchmidtQC)
force-pushed
the
fix/monotonic-dropped-first-basis
branch
from
October 28, 2026 14:15
905b756 to
11be5ed
Compare
|
|
||
|
|
||
| def _drops_first_basis(factor_name: str) -> bool: | ||
| """Whether ``factor_name`` is a ``bs()`` call without ``include_intercept``.""" |
There was a problem hiding this comment.
Do you mind expanding a bit on the docstring or inline comments? I would like to understand in plain english what the conditions are doing.
Contributor
Author
There was a problem hiding this comment.
Fair point. I updated the docstring and also made the body a bit more legible. Unfortunately, there are still quite a number of checks necessary to catch all cases.
1 task done
Member
|
I didn't like the fragile parsing of this PR. I looked at how other libraries dealt with this issue and it seems that passing a flag through tabmat was almost all it took. See #1025 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1021.
When the first column of a monotonic term is dropped, its coefficient is implicitly zero, but only the remaining coefficients were ordered. This happens for
bs()withoutinclude_intercept=Trueand for categoricals withdrop_first=True. If the first remaining coefficient had the wrong sign, the fitted effect was not monotone, even thoughcoef_looked ordered.This PR adds one row per group to the constraints,
sign * c_first >= 0, for such terms. For the example in #1021, an "increasing" constraint on decreasing data now gives a flat fit, as expected.Changes:
_formula.py: detect dropped first columns (defaultbs()calls and reduced-rank factors) and add the extra constraint.bs()andC()withdrop_first=True.monotonic_constraintsin theGeneralizedLinearRegressorandGeneralizedLinearRegressorCVdocstrings.BonusMaluseffect, constrained vs unconstrained. The plot shows that ordered coefficients are sufficient but not necessary for a monotone spline, so the constraint can bind even when the unconstrained fit is already monotone.The monotonic benchmark (glum vs pygam) is unchanged: it uses
cr(), which keeps all columns.A related bug found while working on this, string categorical levels being sorted alphabetically, is tracked separately in #1022.
Checklist
CHANGELOG.rstentry