-
Notifications
You must be signed in to change notification settings - Fork 4
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
Update Survival Plots #208
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @gowerc yeah sounds good!
@danielinteractive - Hmm only thing is it looks like the Options are:
Though for |
We could also add these features to |
Hey @danielinteractive , This is now ready for review. Apologies for the delay the documentation and testing ended up being more involved that I was expecting. That being said it should be all ready now (pending any bugs found by the cicd testing) |
Code Coverage Summary
Diff against main
Results for commit: 642e073 Minimum allowed coverage is ♻️ This comment has been updated with latest results |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thx Craig, just a few minor things below
Hey @danielinteractive , Think I addressed all your comments. I also changed the documentation for the |
Closes #200
@danielinteractive - Still a work in progress but just wanted to share my current state to get any initial feedback. Currently I've delegated the process of generating the data to a dedicated predict function so that users can return the underlying data if they want it e.g:
The autoplot function is now essentially just a thin wrapper around this e.g.
I still need to re-implement the KM option and need to update all the unit tests accordingly.