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

Revamp survival analysis interface #842

Merged
merged 27 commits into from
Jan 10, 2025
Merged

Revamp survival analysis interface #842

merged 27 commits into from
Jan 10, 2025

Conversation

aGuyLearning
Copy link
Collaborator

@aGuyLearning aGuyLearning commented Dec 20, 2024

Fixes #840

This pull request includes refactoring and enhancements to the survival analysis tools, particularly focusing on the Cox Proportional Hazards (CoxPH), Weibull Accelerated Failure Time (Weibull AFT), and Log-Logistic Accelerated Failure Time (Log-Logistic AFT) models. The changes improve the flexibility and usability of these models by adding new parameters and restructuring the code.

Refactoring and Enhancements:

  • Code Refactoring:

    • Split the _regression_model function into two separate functions: _regression_model_data_frame_preparation and _regression_model_populate_adata.
  • Cox Proportional Hazards Model:

    • Added new parameters to the cox_ph function to provide more control over the model fitting process, such as inplace, key_added_prefix, alpha, label, baseline_estimation_method, penalizer, l1_ratio, strata, n_baseline_knots, knots, breakpoints, weights_col, cluster_col, robust, formula, batch_mode, show_progress, initial_point, and fit_options.
  • Weibull Accelerated Failure Time Model:

    • Enhanced the weibull_aft function with additional parameters similar to those added to the cox_ph function.
  • Log-Logistic Accelerated Failure Time Model:

    • Updated the log_logistic_aft function to include new parameters.
  • Testing Adjustments:

    • Modified the _sa_func_test method in tests/tools/test_sa.py to accommodate the updated function signatures, ensuring that tests pass with the new parameters.

@aGuyLearning aGuyLearning linked an issue Dec 20, 2024 that may be closed by this pull request
24 tasks
@aGuyLearning aGuyLearning requested a review from eroell December 20, 2024 10:36
@aGuyLearning
Copy link
Collaborator Author

This is not a finished PR, but a starting point for a discussion. The Weibull_aft and log_logistic_aft model summaries include multi index rows. The question is, if we can reduce them and how that works with the limitations of the adata.var.

We were hoping to include all the summary data into the adata, so as to have a similar style as scanpy.

@aGuyLearning aGuyLearning marked this pull request as draft January 7, 2025 17:05
@Zethson Zethson changed the title Enhancement/issue 840 Revamp survival analysis interface Jan 8, 2025
@aGuyLearning
Copy link
Collaborator Author

aGuyLearning commented Jan 8, 2025

  • Add model result dataframe to .uns
    • Add key name of .uns to be added to the function arguments ( default: function name )
    • PLOT: Search for function name on default
    • PLOT: Take information from .uns object

Do this for:

  • cox_ph
  • weibull_aft
  • log_logistic_aft

@aGuyLearning aGuyLearning marked this pull request as ready for review January 8, 2025 13:30
@eroell
Copy link
Collaborator

eroell commented Jan 8, 2025

Add model result dataframe to .uns

just a quick write-down of our offline discussion:

indeed, I think this is the best way to go from here if we do want to offer nice plots as request in the linked issue.

It is required that we store the survival analysis results in the adata object to produce corresponding plots with the functional API like ep.pl.<fancy_plot>(adata, ...).

However it can't be stored in adata.var, as in addition to the covariates, the models typically include an intercept term, making the model fit results to be of length len(adata.var.columns) + 1.

storing the summary results of the fitter as a very basic DataFrame in adata.uns is the most straightforward way in this case imo

tests/tools/test_sa.py Show resolved Hide resolved
ehrapy/tools/_sa.py Outdated Show resolved Hide resolved
ehrapy/tools/_sa.py Outdated Show resolved Hide resolved
ehrapy/tools/_sa.py Outdated Show resolved Hide resolved
ehrapy/tools/_sa.py Outdated Show resolved Hide resolved
ehrapy/tools/_sa.py Outdated Show resolved Hide resolved
ehrapy/tools/_sa.py Outdated Show resolved Hide resolved
ehrapy/tools/_sa.py Show resolved Hide resolved
@eroell
Copy link
Collaborator

eroell commented Jan 8, 2025

Allows to pass keywords as requested in #744. Does not finish off this issue though, as ideally also show in a notebook tutorial later what effect regularization has.

@eroell eroell self-requested a review January 8, 2025 22:17
@eroell eroell requested a review from Zethson January 8, 2025 22:17
@eroell
Copy link
Collaborator

eroell commented Jan 8, 2025

From my side good. @Zethson, request your review here as this a quite specific choice we're making here.
See comment my comment a quick write-down of our offline discussion above about the rationale behind this choice.
If you have objections, we're interested to hear them

Copy link
Member

@Zethson Zethson left a comment

Choose a reason for hiding this comment

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

Great! Just minor points.

ehrapy/tools/_sa.py Show resolved Hide resolved
ehrapy/tools/_sa.py Outdated Show resolved Hide resolved
ehrapy/tools/_sa.py Outdated Show resolved Hide resolved
ehrapy/tools/_sa.py Outdated Show resolved Hide resolved
ehrapy/tools/_sa.py Outdated Show resolved Hide resolved
ehrapy/tools/_sa.py Outdated Show resolved Hide resolved
ehrapy/tools/_sa.py Outdated Show resolved Hide resolved
ehrapy/tools/_sa.py Outdated Show resolved Hide resolved
ehrapy/tools/_sa.py Outdated Show resolved Hide resolved
tests/tools/test_sa.py Show resolved Hide resolved
@eroell eroell merged commit b2b8e40 into main Jan 10, 2025
11 checks passed
@eroell eroell deleted the enhancement/issue-840 branch January 10, 2025 16:18
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.

Update survival analysis models
3 participants