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 add GEE to genmod #1255

Merged
merged 57 commits into from Dec 19, 2013

Conversation

Projects
None yet
4 participants
@josef-pkt
Copy link
Member

commented Dec 19, 2013

this is a rebased version of PR #928 by @kshedden
see #928 for discussion
additional cleanups from PR #1215 by @vincentarelbundock

I will do another rebase and force push just before merging.

The PR doesn't have any example scripts and examples need to be extracted from unittests or simulations.
I haven't tried building the docs yet. (master has currently already many doc build warnings.)
No systematic testing across python and dependency versions yet.

This is a very large enhancement, and followup review and code adjustments are to be expected.

@josef-pkt

This comment has been minimized.

Copy link
Member Author

commented Dec 19, 2013

@kshedden @vincentarelbundock
Do you have any code relying on this branch?
I'm planning to do another rebase, which will make any branches based on this branch difficult to merge.

I'm planning to merge this within a few hours, after another brief look and running some examples.

@vincentarelbundock

This comment has been minimized.

Copy link
Contributor

commented Dec 19, 2013

I don't. Go ahead.

@coveralls

This comment has been minimized.

Copy link

commented Dec 19, 2013

Coverage Status

Coverage remained the same when pulling 6affdc6 on gee_rebased into 01fd4a2 on master.


top_left = [('Dep. Variable:', None),
('Model:', None),
('Method:', ['Generalized Estimating Equations']),

This comment has been minimized.

Copy link
@josef-pkt

josef-pkt Dec 19, 2013

Author Member

name is too long, makes top table too wide
shorten or break into two lines

return self.dep_params**np.abs(idx[:, None] - idx[None, :]), True


def summary(self):

This comment has been minimized.

Copy link
@josef-pkt

josef-pkt Dec 19, 2013

Author Member

some short summary have print statement instead of returning text
code style and py3 syntax error

effects_idx, const_idx = _get_const_index(exog)

if dummy:
_check_discrete_args(at, method)

This comment has been minimized.

Copy link
@josef-pkt

josef-pkt Dec 19, 2013

Author Member

pylint not defined, same for _get_dummy_index and _get_count_index
missing imports?
missing unit tests

This comment has been minimized.

Copy link
@josef-pkt

josef-pkt Dec 19, 2013

Author Member

DONE


import statsmodels.genmod.families.varfuncs as varfuncs
from statsmodels.genmod.families.links import Link
from statsmodels.genmod.families import Family

This comment has been minimized.

Copy link
@josef-pkt

josef-pkt Dec 19, 2013

Author Member

import should go on top

from statsmodels.discrete.discrete_margins import \
_get_margeff_exog, _get_const_index, _check_margeff_args, \
_effects_at, margeff_cov_with_se, _check_at_is_all, \
_transform_names

This comment has been minimized.

Copy link
@josef-pkt

josef-pkt Dec 19, 2013

Author Member

imports should be on top, even if they belong just to this part

super(GEEResults, self).__init__(model, params,
normalized_cov_params=cov_params, scale=scale)

def standard_errors(self, covariance_type="robust"):

This comment has been minimized.

Copy link
@josef-pkt

josef-pkt Dec 19, 2013

Author Member

follow-up to PR: consistent naming for covariance options across models #1158

Arguments:
----------
covariance_type : string
One of "robust", "naive", or "bias reduced". Determines

This comment has been minimized.

Copy link
@josef-pkt

josef-pkt Dec 19, 2013

Author Member

"naive" versus "nonrobust" another naming convention that we need


results.fit_history = self.fit_history
results.naive_covariance = ncov
results.robust_covariance_bc = bc_cov

This comment has been minimized.

Copy link
@josef-pkt

josef-pkt Dec 19, 2013

Author Member

do we need to calculate always all three cov, bncov and bc_cov

elif covariance_type == "naive":
return np.sqrt(np.diag(self.naive_covariance))
elif covariance_type == "bias_reduced":
return np.sqrt(np.diag(self.robust_covariance_bc))

This comment has been minimized.

Copy link
@josef-pkt

josef-pkt Dec 19, 2013

Author Member

my guess is that this doesn't get the wald_test to use the desired covariance.
cov_params should use the desired covariance_type
needs review and checking.

This comment has been minimized.

Copy link
@josef-pkt

josef-pkt Dec 19, 2013

Author Member

Note: my changes to using default covariances as merged into master is more recent than this code, and we still need to review consistent handling across models.



if __name__ == '__main__':

This comment has been minimized.

Copy link
@josef-pkt

josef-pkt Dec 19, 2013

Author Member

Is this part used or just for development?
I would like to empty all if __name__ == '__main__' code in the main modules. Usually I put them into an ex_modulname.py script in the examples folder inside the source tree.

This comment has been minimized.

Copy link
@josef-pkt

josef-pkt Dec 19, 2013

Author Member

my mistake, this is in a simulation file, the main module doesn't have a main

@josef-pkt

This comment has been minimized.

Copy link
Member Author

commented Dec 19, 2013

gee_gaussian_simulation_check.py is outdated dparams rename and uses standard_errors as attribute not callable.

@coveralls

This comment has been minimized.

Copy link

commented Dec 19, 2013

Coverage Status

Coverage remained the same when pulling bcacbdd on gee_rebased into 01fd4a2 on master.

@josef-pkt

This comment has been minimized.

Copy link
Member Author

commented Dec 19, 2013

predict requires offset

@coveralls

This comment has been minimized.

Copy link

commented Dec 19, 2013

Coverage Status

Coverage remained the same when pulling e2f5191 on gee_rebased into 01fd4a2 on master.

@josef-pkt

This comment has been minimized.

Copy link
Member Author

commented Dec 19, 2013

rebased and force pushed

@coveralls

This comment has been minimized.

Copy link

commented Dec 19, 2013

Coverage Status

Coverage remained the same when pulling d78db20 on gee_rebased into 0b5ed74 on master.

@josef-pkt

This comment has been minimized.

Copy link
Member Author

commented Dec 19, 2013

TravisCI came back green, merging

@kshedden Thanks a lot, that's the biggest new model in some time.
(Still some work left to do.)

josef-pkt added a commit that referenced this pull request Dec 19, 2013

Merge pull request #1255 from statsmodels/gee_rebased
ENH add GEE to genmod closes #928

@josef-pkt josef-pkt merged commit 5a731a3 into master Dec 19, 2013

1 check passed

default The Travis CI build passed
Details

@josef-pkt josef-pkt deleted the gee_rebased branch Dec 30, 2013

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.