Skip to content

Remove intercept for a bare "+ -1" / "+ 0", matching "- 1" (fixes #158)#267

Open
deepakganesh78 wants to merge 1 commit into
pydata:masterfrom
deepakganesh78:fix/issue158-plus-neg1-intercept
Open

Remove intercept for a bare "+ -1" / "+ 0", matching "- 1" (fixes #158)#267
deepakganesh78 wants to merge 1 commit into
pydata:masterfrom
deepakganesh78:fix/issue158-plus-neg1-intercept

Conversation

@deepakganesh78

Copy link
Copy Markdown

This fixes #158.

Root cause

_eval_binary_plus only propagated an added intercept from its right operand (via the intercept flag); it ignored the right operand's intercept_removed flag entirely, keeping only the left operand's intercept state.

So a + b - 1 removes the intercept (the removal lives on the left operand), but a + b + -1 did not — even though the docs imply that appending + -1 is equivalent to appending - 1. The maintainer confirmed in the issue that this is a bug and that the successor, Formulaic, treats a + b + -1 as a + b.

Fix

When the right operand of a binary + is a bare intercept literal (a chain of unary +/- ending directly in 0/1, as in -1), honor its intercept removal — mirroring the existing direct + 0 (ZERO) handling just above it.

A small _is_bare_intercept() helper inspects the parse node so that removal is not propagated out of a compound/parenthesized expression such as 1 + (a + 0), which keeps the intercept by design (covered by the existing ~ 1 + (a + 0) test, which still passes).

Behavior after fix

a + b - 1      -> ['a', 'b']
a + b + -1     -> ['a', 'b']   # was ['Intercept', 'a', 'b']
a + b + 0      -> ['a', 'b']   # was ['Intercept', 'a', 'b']
a + -1 + 1     -> ['Intercept', 'a']   # re-adding still works
0 + a          -> ['a']        # unchanged
1 + (a + 0)    -> ['Intercept', 'a']   # unchanged, by design

Tests

Added regression cases to _eval_tests and a changelog entry. Full suite is green:

147 passed, 1 skipped

(scipy installed so the spline tests run.)

Root cause: _eval_binary_plus only propagated an *added* intercept from
its right operand (via the `intercept` flag); it ignored the right
operand's `intercept_removed` flag entirely, keeping only the left
operand's intercept state. So "a + b - 1" removed the intercept (removal
lives on the left) but "a + b + -1" did not, even though the docs imply
appending "+ -1" is equivalent to appending "- 1".

Fix: when the right operand of a binary "+" is a *bare* intercept literal
(a chain of unary +/- ending directly in 0/1, as in "-1"), honor its
intercept removal, mirroring the existing direct "+ 0" (ZERO) handling.
A new _is_bare_intercept() helper inspects the parse node so that removal
is NOT propagated out of a compound/parenthesized expression such as
"1 + (a + 0)", which keeps the intercept by design (existing test).

Adds regression cases to _eval_tests and a changelog entry.

Fixes pydata#158

@bashtage bashtage left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be appropriate to warn and let the user know since they can easily fix it.

Comment thread patsy/desc.py
# "+ 0" (ZERO) case handled above. We deliberately do *not*
# propagate intercept removal out of a compound expression such as
# "1 + (a + 0)", which keeps the intercept by design.
return IntermediateExpr(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my opinion, this is a "won't fix" type of bug given where the project is. I think a warning would be OK, though. To fix this, one would need to go through a reasonably long deprecation cycle since this is a change to existing code, even if the original implementation is not great. Something that explains to the user what happens here and then tells the user how they can fix it (by slightly altering their formula) is better than a silent behaviour change.

statsmodels will be out with formulaic support next month, so this is moving to lower importance.

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.

"+ -1" equivalence with "-1" in patsy formula?

2 participants