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

ENH: Allow start_params in GLM #1603

Merged
merged 4 commits into from Apr 18, 2014

Conversation

Projects
None yet
3 participants
@jseabold
Copy link
Member

commented Apr 18, 2014

No description provided.

@@ -338,7 +338,8 @@ def predict(self, params, exog=None, linear=False):
return self.family.fitted(np.dot(exog, params) + exposure + \
offset)

def fit(self, maxiter=100, method='IRLS', tol=1e-8, scale=None):
def fit(self, maxiter=100, method='IRLS', tol=1e-8, scale=None,
start_params=None):

This comment has been minimized.

Copy link
@josef-pkt

josef-pkt Apr 18, 2014

Member

we don't guarantee keywords as positional, move start_params to second place, after self

This comment has been minimized.

Copy link
@jseabold

jseabold Apr 18, 2014

Author Member

News to me. Ok.

@josef-pkt

This comment has been minimized.

Copy link
Member

commented Apr 18, 2014

I think we should also adjust the fit history with the start_params
history = dict(params = [None, None], deviance=[np.inf,dev])

@jseabold

This comment has been minimized.

Copy link
Member Author

commented Apr 18, 2014

Should I give this a pep-8 scrubbing in this PR?

@josef-pkt

This comment has been minimized.

Copy link
Member

commented Apr 18, 2014

Should I give this a pep-8 scrubbing in this PR?

Yes no problem (just a separate commit). There are no outstanding PRs for this module AFAIR.

@josef-pkt

This comment has been minimized.

Copy link
Member

commented Apr 18, 2014

A better unit test: check convergence requires only <=1 iteration when starting from the optimal parameters. I don't think we need to test everything again.
(some developers complain about the time it takes to run the test suite.)

@coveralls

This comment has been minimized.

Copy link

commented Apr 18, 2014

Coverage Status

Changes Unknown when pulling f510afa on jseabold:glm-start-params into * on statsmodels:master*.

@jseabold

This comment has been minimized.

Copy link
Member Author

commented Apr 18, 2014

Time to copy and paste a test was more important here.

|10 $ nosetests test_glm.py:Test_start_params
.............
----------------------------------------------------------------------
Ran 13 tests in 0.008s

OK
@coveralls

This comment has been minimized.

Copy link

commented Apr 18, 2014

Coverage Status

Changes Unknown when pulling 77851b8 on jseabold:glm-start-params into * on statsmodels:master*.

@jseabold

This comment has been minimized.

Copy link
Member Author

commented Apr 18, 2014

I think this should be mu = self.family.fitted(np.dot(exog, start_params)) so that it goes through the inverse transform first and we're not giving linear predictors to the family that expects transformed mu. This allows us to fit problems like #1604 by giving start_params, though it doesn't solve the bad default start_params and the silently failing fit.

@josef-pkt

This comment has been minimized.

Copy link
Member

commented Apr 18, 2014

I think you are right about mu = self.family.fitted(np.dot(exog, start_params))
I never remember which is mu and which is eta. (greek one letter names)
also, a test that doesn't check the number of iterations might not catch this.

@jseabold

This comment has been minimized.

Copy link
Member Author

commented Apr 18, 2014

I added the test from #1604 which fails without the correct transform. I'm not really sure what else to do about the degenerate case here where the probability on one of the observations is pushed to 1 or 0.

This should be good to merge, and I'll leave that as an open problem. Hilbe comments in that blog post that they have an improved IRLS algorithm that doesn't fail, but I don't know what it is.

@coveralls

This comment has been minimized.

Copy link

commented Apr 18, 2014

Coverage Status

Changes Unknown when pulling d52f144 on jseabold:glm-start-params into * on statsmodels:master*.

jseabold added a commit that referenced this pull request Apr 18, 2014

Merge pull request #1603 from jseabold/glm-start-params
ENH: Allow start_params in GLM

@jseabold jseabold merged commit d3c2add into statsmodels:master Apr 18, 2014

1 check passed

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

@jseabold jseabold deleted the jseabold:glm-start-params branch Apr 18, 2014

@josef-pkt josef-pkt added the PR label Aug 11, 2014

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

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.