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: PHReg formula improvements #1954

Merged
merged 2 commits into from Sep 26, 2014

Conversation

Projects
None yet
4 participants
@kshedden
Copy link
Contributor

commented Sep 6, 2014

This PR allows extra arguments of array type to be passed to the PHReg from_formula method as variable names. The data are read out of the same data object that is used for the formula.

@coveralls

This comment has been minimized.

Copy link

commented Sep 6, 2014

Coverage Status

Coverage increased (+0.01%) when pulling 2ac71f3 on kshedden:phreg_formula into 9ce1605 on statsmodels:master.

@josef-pkt

This comment has been minimized.

Copy link
Member

commented Sep 18, 2014

looks good

I'm not sure or I haven't checked if the argument names are already "standard"

@classmethod
def from_formula(cls, formula, data, status=None, entry=None,
strata=None, offset=None, subset=None,
ties='breslow', missing='drop', *args, **kwargs):

This comment has been minimized.

Copy link
@jseabold

jseabold Sep 21, 2014

Member

I don't feel particularly strongly about it but we have missing='none' as the default everywhere. The idea is not to do any unnecessary computations unless asked for.

This comment has been minimized.

Copy link
@josef-pkt

josef-pkt Sep 21, 2014

Member

Since patsy 1.2 (IIRC) we have missing='drop' as default in from_formula because we don't prevent patsy from dropping missing rows.
We still have the open issues about conflicts between patsy's and our missing value handling.

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

assert(len(md.endog) == 185)
assert(len(md.status) == 185)
assert(all(md.exog.shape == np.r_[185,4]))
assert_allclose(len(md.endog), 185)

This comment has been minimized.

Copy link
@josef-pkt

josef-pkt Sep 25, 2014

Member

integers can be tested with assert_equal (no floating point errors possible)

@josef-pkt

This comment has been minimized.

Copy link
Member

commented Sep 25, 2014

@kshedden @jseabold
About the name again. I was reading a survey article (for actuarians) about parametric proportional hazard models. I don't have a good overview of what other related models we might get.
Is PHReg too general as a name without Cox or Partial in the name?
Frailty models, parametric PH models, AFT accelerated failure time, ... I don't know how they all fit together.

We can keep PHReg for now as the name, but we might have to rename if we get additional similar models in future.

@jseabold

This comment has been minimized.

Copy link
Member

commented Sep 26, 2014

Ideally, we get the docstring for free in from_formula so we don't have to have it twice, but that will have to wait. Merging.

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

Merge pull request #1954 from kshedden/phreg_formula
ENH: PHReg formula improvements

@jseabold jseabold merged commit 9f5b54d into statsmodels:master Sep 26, 2014

2 checks passed

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

@kshedden kshedden deleted the kshedden:phreg_formula branch Sep 27, 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.