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 api for covstruct. Clear __init__. Closes #1917. #1981

Merged
merged 4 commits into from Sep 24, 2014

Conversation

Projects
None yet
4 participants
@jseabold
Copy link
Member

commented Sep 21, 2014

closes #1917

@@ -9,6 +9,7 @@
from .genmod.generalized_linear_model import GLM
from .genmod.generalized_estimating_equations import GEE
from .genmod import families
from .genmod.dependence_structures import api as covariance

This comment has been minimized.

Copy link
@josef-pkt

josef-pkt Sep 21, 2014

Member

covariance is too generic at the top level (could be any kind of covariance estimator)
I was just thinking about this because I use the full import path in the gee.rst example

maybe covstruct like the module name (or cov_struct ? guess not)
initially I was thinking of sm.genmod.covstruct
(I couldn't make up my mind)

This comment has been minimized.

Copy link
@jseabold

jseabold Sep 21, 2014

Author Member

Ok well let's make a decision. I'll change it to cov_struct and import it in the genmod namespace though I don't think it's particularly better than covariance for any reason.

This comment has been minimized.

Copy link
@josef-pkt

josef-pkt Sep 21, 2014

Member

covstruct: The code is currently only useful for GEE. It's tightly integrated with the GEE class
We need more general covariance estimators, for example Kevin add a newey west covariance similar to the inner part of the sandwiches.
So I'd like to reserve any generic covariance name for when we have useful standalone functions.

@josef-pkt josef-pkt added this to the 0.6 milestone Sep 23, 2014

from .genmod.generalized_linear_model import GLM
from .genmod.generalized_estimating_equations import GEE
from .genmod import families
from .genmod.api import *

This comment has been minimized.

Copy link
@josef-pkt

josef-pkt Sep 23, 2014

Member

we don't do import * into the main api module space (1), even if we have all of it.

(1) at least we haven't done it so far, and should stick with explicit imports

This comment has been minimized.

Copy link
@jseabold

jseabold Sep 23, 2014

Author Member

I just figured this is what the api files are for. We've already done the explicit imports in the lower-level api, so why type it twice?

This comment has been minimized.

Copy link
@josef-pkt

josef-pkt Sep 23, 2014

Member

(by email, trying to recover from temporary power outage but browsers don't
work properly for github)

First, we never do import * anymore, it's not informative, automatic
tools don't work, and I don't see what we have added to the api.

Second, the content of the subpackage api are not imported into the
statsmodels.api. We use them to create the next layer of import paths with
content that is not entirely in the top level api.
See from .stats import api as stats and similar.

This comment has been minimized.

Copy link
@jseabold

jseabold Sep 23, 2014

Author Member

On Tue, Sep 23, 2014 at 11:58 AM, Josef Perktold notifications@github.com
wrote:

In statsmodels/api.py:

@@ -6,9 +6,7 @@
from .regression.linear_model import OLS, GLS, WLS, GLSAR
from .regression.quantile_regression import QuantReg
from .regression.mixed_linear_model import MixedLM
-from .genmod.generalized_linear_model import GLM
-from .genmod.generalized_estimating_equations import GEE
-from .genmod import families
+from .genmod.api import *

(by email, trying to recover from temporary power outage but browsers
don't work properly for github) First, we never do import * anymore, it's
not informative, automatic tools don't work, and I don't see what we have
added to the api. Second, the content of the subpackage api are not
imported into the statsmodels.api. We use them to create the next layer of
import paths with content that is not entirely in the top level api. See
from .stats import api as stats and similar.

It's already gone.

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

Closed

release 0.6 #912

11 of 25 tasks complete
@jseabold

This comment has been minimized.

Copy link
Member Author

commented Sep 23, 2014

I'm starting to think I shouldn't nuke the __init__.py in dependence_structures so then it will be more like what we have for families now, which is convenient. I'm not entirely convinced that we even need a dependence_structures directory to hold one file.

@jseabold

This comment has been minimized.

Copy link
Member Author

commented Sep 23, 2014

@kshedden Do you see the dependence_structures folder filling out? If not I'm inclined to move covstruct.py to ../cov_struct.py. Thoughts?

@jseabold

This comment has been minimized.

Copy link
Member Author

commented Sep 23, 2014

I went ahead and did it. We now have them available in the top-level namespace just like the families. So we can do sm.cov_struct.Independence.

@jseabold jseabold force-pushed the jseabold:cov_struct-import branch from 8390cd6 to 3184054 Sep 23, 2014

@coveralls

This comment has been minimized.

Copy link

commented Sep 23, 2014

Coverage Status

Coverage decreased (-0.0%) when pulling 3184054 on jseabold:cov_struct-import into 2fcc5ae on statsmodels:master.

@kshedden

This comment has been minimized.

Copy link
Contributor

commented Sep 23, 2014

Seems OK to me. I'll leave these sorts of decisions up to you guys.

Eventually the covstruct code may be split across a few files. I need to cythonize some of the global odds ratio calculations and add more dependence structures for categorical data, which can get quite intricate.

from .genmod.generalized_estimating_equations import GEE
from .genmod import families
from .genmod import api as genmod
from .genmod.api import GLM, GEE, families, cov_struct

This comment has been minimized.

Copy link
@josef-pkt

josef-pkt Sep 23, 2014

Member

minor: I like the long imports from the module directly

@josef-pkt

This comment has been minimized.

Copy link
Member

commented Sep 23, 2014

Looks good to me now.

@kshedden You will need to adjust the imports in your notebooks after this is merged

@jseabold

This comment has been minimized.

Copy link
Member Author

commented Sep 24, 2014

I think the entry points are right at least. We can adjust directory structure any time it's needed. Rebasing and merging.

@jseabold jseabold force-pushed the jseabold:cov_struct-import branch from 3184054 to 55f55c1 Sep 24, 2014

jseabold added a commit that referenced this pull request Sep 24, 2014

Merge pull request #1981 from jseabold/cov_struct-import
ENH: Add api for covstruct. Clear __init__. Closes #1917.

@jseabold jseabold merged commit e27e5dc into statsmodels:master Sep 24, 2014

2 checks passed

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

@jseabold jseabold deleted the jseabold:cov_struct-import branch Sep 24, 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.