Add user control over what happens if a constant is already present. #2093

Merged
merged 9 commits into from Nov 17, 2014

Projects

None yet

2 participants

@jseabold
Member

Fix for #2043.

@jseabold
Member

@josef-pkt Do you have an opinion about changing the default behavior here to raise if a constant is already present? Alternatively, I can leave the current behavior and make fixes downstream, though I kinda convinced myself that the current behavior is a wart and working around it probably isn't the right thing to do.

@josef-pkt
Member

In general I think it's fine. I'm finishing up some unit tests so I can open a PR, and can look at it more carefully in an hour or so.

I'm not sure about the default, 'raise' or skip/ignore which has been our general policy so far

  • I think using np.ptp is better (cheaper) than using np.var.
  • In general we want to move to a more careful handling of singular exog with extra options. In this case this might not go far enough when we want to also avoid adding a constant when there is an implicit constant.
  • The option in the helper function is fine, but when we call them internally, then we need proper "singular" handling in whatever the calling model is. (I haven't looked at #2043 yet)
  • same change would apply to similar helper functions like add_trend, ... ?
@jseabold
Member

I just want to close #2043 with this. SIngular exog/highly collinear term handling is not addressed here. The problem is with trend='c' in VAR and there's already a constant in the model. This raises a better error message sooner rather than later. The main change here is that people relying on the do nothing behavior of add_constant will now get an error raised. I'm assuming this is a very small number of people, if any. add_trend calls add_constant internally, so that's covered.

@josef-pkt
Member

I understand the limited change here, but I don't want to change the default again when we add full "singular" handling because then we change the behavior several times across releases.

If we know more changes are coming, then I'd rather not do incremental API changes.
For example, we could keep the default at ignore in the helper function and use the raise option when it is called in #2043

@jseabold
Member

Ok, yeah. Seems like a fair enough concern and easy enough to avoid
changing the default until we know what more full-featured reduced rank
handling will look like.

@josef-pkt
Member

Actually, I think the singular check (has already a constant) is relatively easy, although maybe not very cheap.
Kevin's original code for checking for implicit constant was just comparing the rank before and after adding the constant. If it doesn't increase by one, then a implicit or explicit constant is already in there.

@jseabold jseabold added this to the 0.6.1 milestone Nov 17, 2014
@jseabold jseabold merged commit b88f500 into statsmodels:master Nov 17, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@jseabold jseabold deleted the jseabold:add-constant-check branch Nov 17, 2014
@jseabold jseabold added a commit that referenced this pull request Dec 2, 2014
@jseabold jseabold Backport PR #2093: Add user control over what happens if a constant i…
…s already present.

Fix for #2043.
b1a1d6a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment