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.

Sign up for GitHub

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

Jump to bottom

Fix scaling in cox ph prediction #295

Merged
merged 2 commits into from
Jun 6, 2017
Merged

Fix scaling in cox ph prediction #295

merged 2 commits into from
Jun 6, 2017

Conversation

CamDavidsonPilon
Copy link
Owner

@CamDavidsonPilon CamDavidsonPilon commented Jun 4, 2017 •
edited
Loading

cc @disimone, can you try your code with this latest branch?

I chose to keep normalisation in the library, but it is all transparent to the user. The reason to keep it is for numeric stability.

Fixes #294

CamDavidsonPilon
CamDavidsonPilon commented Jun 4, 2017
View reviewed changes
tests/test_estimation.py
@@ -925,12 +927,79 @@ def test_hazard_works_as_intended_with_strata_against_R_output(self, rossi):
npt.assert_almost_equal(cp.baseline_cumulative_hazard_[(0, 0, 0, 0)].ix[[14, 35, 37, 43, 52]].values, [0.076600555, 0.169748261, 0.272088807, 0.396562717, 0.396562717], decimal=2)
npt.assert_almost_equal(cp.baseline_cumulative_hazard_[(0, 0, 0, 1)].ix[[27, 43, 48, 52]].values, [0.095499001, 0.204196905, 0.338393113, 0.338393113], decimal=2)

def test_baseline_survival_is_the_same_indp_of_location(self, regression_dataset):
Copy link
Owner Author

Choose a reason for hiding this comment

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

these tests should convince you that the internal normalization is transparent to the user.

@CamDavidsonPilon CamDavidsonPilon merged commit 8e3272a into master Jun 6, 2017
@CamDavidsonPilon CamDavidsonPilon deleted the fix-scaling-in-cox-ph-prediction branch June 6, 2017 03:05
@disimone
Copy link

Hi,

sorry for the late reply, I have been offline for a few days. I am happy to test this. Is the new code available in a release?

Andrea.

@CamDavidsonPilon
Copy link
Owner Author

Hey, yes it should be as 0.10.1

@disimone
Copy link

disimone commented Jun 14, 2017 •
edited
Loading

Hi,

things look a lot better now, thanks for the fix. I noticed, however, that the plot() function of the KM fitter is apparently broken.

Any attempt to use it (kmf.plot(ax=my_ax)) results in a quite spectacular stack trace, but I think the culprit is in

KeyError                                  Traceback (most recent call last)
~/Envs/DSpy3/lib/python3.5/site-packages/matplotlib/colors.py in to_rgba(c, alpha)
    140     try:
--> 141         rgba = _colors_full_map.cache[c, alpha]
    142     except (KeyError, TypeError):  # Not in cache, or unhashable.

KeyError: (0.2980392156862745, None)

Plotting by hand the survival function works, so I guess the problem is really in your plotting utility.

Thanks for your help,

Andrea.

@disimone
Copy link

I investigated the plotting problem. If I manually specify a color, the error disappears. this is, however, annoying, since I want to plot several lines on the same axes and let matplotlib choose the different colors automatically...

@CamDavidsonPilon
Copy link
Owner Author

ah, strange, may be a matplotlib 2.x compatibility. Can you post your version of matplotlib here please?

@disimone
Copy link

hi, I am using matplotlib 2.0.2

@CamDavidsonPilon
Copy link
Owner Author

This kmf plot bug was fixed in 0.11.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@CamDavidsonPilon @disimone