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

Feature/add patsy formulas #60

Closed
wants to merge 48 commits into from
Closed

Feature/add patsy formulas #60

wants to merge 48 commits into from

Conversation

jburos
Copy link
Member

@jburos jburos commented Feb 27, 2017

Some code refactoring in order to support using patsy formulas for LHS as well as RHS spec. Closes #52

Models can now use the following syntax:

survivalstan.fit_stan_survival_model(
    formula = 'surv(event_status=end_failure, time=t) ~ age + sex',
    data = d,
    model = survivalstan.models.exp_survival_model,
     ...
)

Optional parameters to surv include group= and subject=.

The previous syntax is still also supported:

survivalstan.fit_stan_survival_model(
    event_col = 'end_failure', 
    time_col = 't',
    formula = ' ~ age + sex',
    ...
) 

Moved a fair amount of data-processing into the survivalstan.formulas module, which extends patsy syntax to support the above. For example, the patsy.ModelDesc module includes the stan-data dict which gets passed to pystan.stan for sampling.

This ipynb tests early versions of this code and may help demonstrate how the elements in formulas.py fit together.

@coveralls
Copy link

coveralls commented Feb 27, 2017

Coverage Status

Coverage decreased (-23.7%) to 58.779% when pulling b69d1d5 on feature/add-patsy-formulas into 4bb29b7 on master.

@coveralls
Copy link

coveralls commented Feb 28, 2017

Coverage Status

Coverage decreased (-36.5%) to 45.978% when pulling 3193432 on feature/add-patsy-formulas into 4bb29b7 on master.

@coveralls
Copy link

coveralls commented Feb 28, 2017

Coverage Status

Coverage increased (+1.2%) to 83.673% when pulling 3193432 on feature/add-patsy-formulas into 4bb29b7 on master.

@coveralls
Copy link

coveralls commented Feb 28, 2017

Coverage Status

Coverage increased (+1.6%) to 84.082% when pulling 1946c51 on feature/add-patsy-formulas into 4bb29b7 on master.

Copy link
Member

@armish armish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a total stan/patsy n00b here, so don't feel like I can CR at a higher level and talk about the functionality, but here are a few suggestions/comments regarding the coding style.

survivalstan/formulas.py Show resolved Hide resolved
survivalstan/formulas.py Show resolved Hide resolved
survivalstan/formulas.py Outdated Show resolved Hide resolved
survivalstan/formulas.py Show resolved Hide resolved
survivalstan/formulas.py Outdated Show resolved Hide resolved
value in pars.items()]))
return(lhs_formula)

def _gen_surv_formula(rhs_formula, event_col, time_col=None,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe pull this and all of the other functions above into the SurvivalStanData since I believe they are ony being used in that context. If you think that would make things crowded for a single class, maybe it is worth implementing a utility class that you init with a formula once then can use for further sanity checks, e.g.:

formula = FormulaChecks(formula)
if formula.has_lhs():
   ...

@coveralls
Copy link

coveralls commented Mar 3, 2017

Coverage Status

Changes Unknown when pulling 2dee964 on feature/add-patsy-formulas into ** on dev**.

Copy link
Member

@julia326 julia326 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some initial comments - let's catch up briefly offline so I can get some background?

survivalstan/formulas.py Show resolved Hide resolved
survivalstan/formulas.py Show resolved Hide resolved
survivalstan/formulas.py Show resolved Hide resolved
survivalstan/formulas.py Show resolved Hide resolved
survivalstan/formulas.py Show resolved Hide resolved
survivalstan/formulas.py Show resolved Hide resolved
self.subject_id = Id('subject')
self.timepoint_id = Id('timepoint')
self.group_id = Id('group')
self._type = None
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add comments explaining what _type and _grouped are?

survivalstan/formulas.py Show resolved Hide resolved
survivalstan/formulas.py Outdated Show resolved Hide resolved
@TariqAHassan
Copy link

@jburos

Any news on this? It would be very cool!

@jburos jburos changed the base branch from master to dev September 5, 2018 20:30
@jburos
Copy link
Member Author

jburos commented Jan 8, 2019

Have migrated this work to PR #77

@jburos jburos closed this Jan 8, 2019
@jburos jburos deleted the feature/add-patsy-formulas branch January 11, 2019 20:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants