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

Bug gee cov type 1906 rebased #1924

Merged
merged 13 commits into from Aug 23, 2014

Conversation

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

josef-pkt commented Aug 21, 2014

rebased version of #1916, makes #1916 obsolete except for comments

one commit was heavily edited during merge conflict with attribute handling in subclasses.
In contrast to Kerby's previous version, I add/copy now all attributes in the subclasses through an argument to the results.__init__. Kerby assigned the attributes after creating the instance, which is too late for the attributes that are required to be available in __init__.

TODO: now subclasses can also be added to the unit test to make sure cov_params_default is correctly set. Done for the cov_params_default test, not added to generic tests in base

I renamed cov_type and the cov_xxx in a not backwards compatible way.
(I needed to fix my noteboook for this two renamings.)

I renamed resid_xxx and sensitivity_params to use postfix qualifiers, but I kept the old names as alias. I didn't adjust the code to use the new names.

Summary of PR

  • fixes cov_type handling #1906 (original PR for this #1916 )
  • fix NominalGEE to work with pandas DataFrames and Series #1931
  • fix predict #1919
  • add results wrapper: fit now return wrapped instance, with pandas for the main results #1904
  • add df_resid so that t_test works #1918
  • rename covariance_type -> cov_type
  • rename resid_xxx and sensitivity_params, with aliased old names
  • more unit tests, all changes are covered

open

  • remove alias names
  • make cov_type calculations lazy #1910
  • review arguments transfer from fit to results, after or if cov_types are calculated through results.
  • verified or regression unit tests for cov_robust_bc`
@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 21, 2014

Coverage Status

Coverage increased (+0.13%) when pulling 3822520 on josef-pkt:bug_gee_cov_type_1906_rebased into e373b72 on statsmodels:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 21, 2014

Coverage Status

Coverage increased (+0.13%) when pulling 3b5c3ac on josef-pkt:bug_gee_cov_type_1906_rebased into e373b72 on statsmodels:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 22, 2014

Coverage Status

Coverage increased (+0.13%) when pulling bffd3a6 on josef-pkt:bug_gee_cov_type_1906_rebased into e373b72 on statsmodels:master.

@josef-pkt

This comment has been minimized.

Copy link
Member Author

josef-pkt commented Aug 22, 2014

this should be about finished now

latest changes:

  • add df_resid
  • fix pandas in NominalGEE
  • add wrappers

still not consistent: residuals and maybe others in results should use postfix qualifier, e.g. resid_centered, ...

@josef-pkt

This comment has been minimized.

Copy link
Member Author

josef-pkt commented Aug 22, 2014

here's my partially updated notebook, running with this branch
http://nbviewer.ipython.org/gist/josef-pkt/4d2862963ba13df5e4ab

@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 22, 2014

Coverage Status

Coverage increased (+0.16%) when pulling a59a0b7 on josef-pkt:bug_gee_cov_type_1906_rebased into e373b72 on statsmodels:master.

@josef-pkt

This comment has been minimized.

Copy link
Member Author

josef-pkt commented Aug 22, 2014

oh fun, pandas compatibility, but in py 3.3 version only

uses pandas 0.13, I used pandas 0.12 for testing
guess: this is a consequence that the results attributes are now pandas data structures because of the wrapper. use results._results for verifying numbers
todo: check pythonxy tomorrow for testing with pandas master.

ERROR: statsmodels.genmod.tests.test_gee.TestGEE.test_compare_OLS
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/travis/miniconda/envs/statsmodels-test/lib/python3.3/site-packages/nose/case.py", line 198, in runTest
self.test(*self.arg)
File "/home/travis/miniconda/envs/statsmodels-test/lib/python3.3/site-packages/statsmodels-0.6.0-py3.3-linux-x86_64.egg/statsmodels/genmod/tests/test_gee.py", line 719, in test_compare_OLS
assert_almost_equal(naive_tvalues, ols.tvalues, decimal=10)
File "/home/travis/miniconda/envs/statsmodels-test/lib/python3.3/site-packages/numpy/testing/utils.py", line 459, in assert_almost_equal
if not (gisfinite(desired) and gisfinite(actual)):
File "/home/travis/miniconda/envs/statsmodels-test/lib/python3.3/site-packages/pandas/core/generic.py", line 676, in __nonzero__
.format(self.__class__.__name__))
ValueError: The truth value of a Series is ambiguous. Use a.empty, a.bool(), a.item(), a.any() or a.all().

@josef-pkt josef-pkt force-pushed the josef-pkt:bug_gee_cov_type_1906_rebased branch from 251c148 to 9d7442e Aug 23, 2014

@coveralls

This comment has been minimized.

Copy link

coveralls commented Aug 23, 2014

Coverage Status

Coverage increased (+0.16%) when pulling 9d7442e on josef-pkt:bug_gee_cov_type_1906_rebased into e373b72 on statsmodels:master.

@josef-pkt

This comment has been minimized.

Copy link
Member Author

josef-pkt commented Aug 23, 2014

replaced by checking either values or the unwrapped results._results, so asserts are numpy only.

@josef-pkt

This comment has been minimized.

Copy link
Member Author

josef-pkt commented Aug 23, 2014

@kshedden I would like to merge this very soon. (We are already getting late for the release, for the Debian deadlines.)

Do you want to review this and check whether the changes are fine with you?

@kshedden

This comment has been minimized.

Copy link
Contributor

kshedden commented Aug 23, 2014

Yes, I've reviewed it. Looks good. Thank you for dealing with this.

On Sat, Aug 23, 2014 at 11:06 AM, Josef Perktold notifications@github.com
wrote:

@kshedden https://github.com/kshedden I would like to merge this very
soon. (We are already getting late for the release, for the Debian
deadlines.)

Do you want to review this and check whether the changes are fine with you?


Reply to this email directly or view it on GitHub
#1924 (comment)
.

@josef-pkt

This comment has been minimized.

Copy link
Member Author

josef-pkt commented Aug 23, 2014

Thanks, merging

josef-pkt added a commit that referenced this pull request Aug 23, 2014

Merge pull request #1924 from josef-pkt/bug_gee_cov_type_1906_rebased
BUG/REF gee cov type and other fixes

@josef-pkt josef-pkt merged commit 957a43e into statsmodels:master Aug 23, 2014

2 checks passed

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

@josef-pkt josef-pkt deleted the josef-pkt:bug_gee_cov_type_1906_rebased branch Aug 23, 2014

@josef-pkt josef-pkt referenced this pull request Aug 23, 2014

Closed

BUG GEE cov type #1916

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.