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

Pickle Serialization for UnivariateFitter models #525

Merged
merged 10 commits into from
Oct 25, 2018

Conversation

dwilson1988
Copy link
Contributor

Essentially I kept with the original design with minor changes. Instead of using factory functions to build the predict/subtract/divide/plot methods, I converted them to base class methods and update the docstrings after fitting. The methods are always present, but raise an exception if fit hasn't been called. Shuffled things around slightly and added a very simple unit test to make sure serialization is working with pickle. Tested on python version 3.6.4, linux.

fixes #524 and #188

@dwilson1988
Copy link
Contributor Author

It passes all tests except a test that simply says "assert false", so I don't think that one is my fault. :)

@@ -299,6 +300,17 @@ def test_typeerror_is_thrown_if_there_is_nans_in_the_event_col(self, univariate_
with pytest.raises(TypeError):
fitter().fit(T, E)

def test_pickle_serialization(self, positive_sample_lifetimes, univariate_fitters):
Copy link
Owner

Choose a reason for hiding this comment

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

👍

@@ -1333,5 +1336,9 @@ def _is_monotonically_decreasing(array):
def next(self):
return self.step_size

def _to_array(x):
Copy link
Owner

Choose a reason for hiding this comment

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

ah neat, I can use this function elsewhere

@CamDavidsonPilon
Copy link
Owner

First look: really clear pattern, and a nice generalisation of the univariate fitters.

return self

def plot_loglogs(self,*args,**kwargs):
Copy link
Owner

Choose a reason for hiding this comment

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

small lint thing: can you add spaces between these args, and the line below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@CamDavidsonPilon
Copy link
Owner

TravisCI is pretty red, but you say tests pass locally?

@dwilson1988
Copy link
Contributor Author

All except test_robust_errors_with_strata_doesnt_break.

assert False

@CamDavidsonPilon
Copy link
Owner

lgtm, really appreciate this contribution, @dwilson1988!

@CamDavidsonPilon CamDavidsonPilon merged commit 17b2931 into CamDavidsonPilon:v0.15.0 Oct 25, 2018
@dwilson1988
Copy link
Contributor Author

Awesome. I needed this, so I hope this works out for everyone else as well!

@CamDavidsonPilon
Copy link
Owner

CamDavidsonPilon commented Oct 25, 2018

FYI I'm hoping to release 0.15 mid-December at the latest, but maybe sooner. Feel free to rip the latest though with pip install git+https://github.com/CamDavidsonPilon/lifelines.git@v0.15.0 (maybe target your commit, incase I break that branch).

Lots of nice changes https://github.com/CamDavidsonPilon/lifelines/blob/17b2931f8e58fc2e4715e3bbcdac00b449bceba9/CHANGELOG.md#0150

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.

None yet

3 participants