Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow offset and exposure to be used together with log link; raise excep... #1635

Merged
merged 1 commit into from May 20, 2014

Conversation

Projects
None yet
3 participants
@kshedden
Copy link
Contributor

commented Apr 24, 2014

...tion if exposure is used when the ink is not the log

@coveralls

This comment has been minimized.

Copy link

commented May 6, 2014

Coverage Status

Coverage remained the same when pulling e625801 on kshedden:glm_offset5 into b52bc09 on statsmodels:master.

@josef-pkt

This comment has been minimized.

Copy link
Member

commented May 20, 2014

I just saw that there is also a bug in predict.
It shouldn't use the stored offset and exposure if exog is given. Should result in shape mismatch if predict exog doesn't have nobs rows.

@kshedden

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2014

Should we add arguments for offset and exposure and/or warn? It seems
dangerous to silently ignore them.

On Tue, May 20, 2014 at 11:33 PM, Josef Perktold
notifications@github.comwrote:

I just saw that there is also a bug in predict.
It shouldn't use the stored offset and exposure if exog is given. Should
result in shape mismatch if predict exog doesn't have nobs rows.


Reply to this email directly or view it on GitHubhttps://github.com//pull/1635#issuecomment-43642175
.

@josef-pkt

This comment has been minimized.

Copy link
Member

commented May 20, 2014

Yes, I think we need to include the offset and exposure arguments in the same way as in discrete_model CountModel.predict.

aside: CountModel.predict uses a user specified offset and exposure, even if the estimation didn't use it. I don't see a problem with this. Could be useful if we want to "manipulate" the prediction.

@kshedden

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2014

Should I add this to the glm_offset5 PR, or start a new one?

On Wed, May 21, 2014 at 12:00 AM, Josef Perktold
notifications@github.comwrote:

Yes, I think we need to include the offset and exposure arguments in the
same way as in discrete_model CountModel.predict.

aside: CountModel.predict uses a user specified offset and exposure, even
if the estimation didn't use it. I don't see a problem with this. Could be
useful if we want to "manipulate" the prediction.


Reply to this email directly or view it on GitHubhttps://github.com//pull/1635#issuecomment-43645725
.

@josef-pkt

This comment has been minimized.

Copy link
Member

commented May 20, 2014

I think I could merge this as is. Only the commit message is wrong.

Then you can start a new branch from updated master

josef-pkt added a commit that referenced this pull request May 20, 2014

Merge pull request #1635 from kshedden/glm_offset5
REF/BUG Allow offset and exposure to be used together with log link, see #1486

@josef-pkt josef-pkt merged commit e0fed32 into statsmodels:master May 20, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@josef-pkt

This comment has been minimized.

Copy link
Member

commented May 20, 2014

merged, thank Kerby.

@josef-pkt josef-pkt added PR labels May 21, 2014

@kshedden kshedden deleted the kshedden:glm_offset5 branch Jun 9, 2014

PierreBdR pushed a commit to PierreBdR/statsmodels that referenced this pull request Sep 2, 2014

Merge pull request statsmodels#1635 from kshedden/glm_offset5
REF/BUG Allow offset and exposure to be used together with log link, see statsmodels#1486
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.