-
Notifications
You must be signed in to change notification settings - Fork 22
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
New Feature: Forestplot for CoxPH model #838
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Hm checking into the failed docs build |
How did you make the option to have this dashed vertical bar before? That seemed quite nice! |
Co-authored-by: Eljas Roellin <[email protected]>
Co-authored-by: Eljas Roellin <[email protected]>
And at the end flexing this feature in the survival notebook would be great.. :) |
sorry, misclicked! |
Medcat no more a doc build dependency; asked about state of Python 3.12 CogStack/MedCAT#510 |
for more information, see https://pre-commit.ci
Co-authored-by: Lukas Heumos <[email protected]>
One a sidenote, there should be a pull request template with checkmarks and some suggested subtitles for the first comment, which we should use for all pull requests ideally. |
ehrapy/plot/_survival_analysis.py
Outdated
|
||
Args: | ||
adata: :class:`~anndata.AnnData` object containing all observations in `.uns`. | ||
uns_key: Key in `.uns` where the CoxPHFitter object is stored. |
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.
Change to a brief description of the result that is stored there; mention and reference the ehrapy function who writes the results into this field
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.
This should be updated or at least synced with the uns_key
argument description, where there is also still the wrong description about the CoxPHFitter object
ehrapy/plot/_survival_analysis.py
Outdated
Args: | ||
adata: :class:`~anndata.AnnData` object containing all observations in `.uns`. | ||
uns_key: Key in `.uns` where the CoxPHFitter object is stored. | ||
labels: List of labels for each coefficient, default uses the index of the coxph.summary. |
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.
what is coxph.summary? (I know, but users should be able to understand what this is doing by just following our documentation)
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.
I think in the updates of the current review request, this is still as it looked when I made this comment? :)
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.
My bad, very sorry, i should first fix it in my editor and then sign of here.
Co-authored-by: Eljas Roellin <[email protected]>
Quick question on the additional functionality with the secondary helper plots "below" the forestplot; are you planning to add them, or are they too unstable/time consuming to set up at this stage? (fine if this is the case of course, could also be a later enhancement!) |
There is no table intended under the forest plot. i believe you mean #742 |
Ah yes, had mixed this up, thanks! |
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.
See 2 re-unresolved comments
… of adata and summary table
PR Checklist
Resolves #743
docs
is updatedThis pull request introduces a new plotting function for Cox Proportional Hazards models and updates related documentation and tests. The most important changes include adding a new plotting function, updating imports, and adding new tests.
New Feature:
cox_ph_forestplot
function to generate forest plots for Cox Proportional Hazards models inehrapy/plot/_survival_analysis.py
.Documentation Updates:
docs/usage/usage.md
to includeplot.cox_ph_forestplot
in the list of available plotting functions.Import Adjustments:
ehrapy/plot/__init__.py
to import the newcox_ph_forestplot
function.ehrapy/plot/_survival_analysis.py
to include necessary modules for the new plotting function.Testing Enhancements:
tests/plot/test_survival_analysis.py
to validate thecox_ph_forestplot
function.tests/_scripts/coxph_forestplot_create_expected.ipynb
to generate expected output for the new plotting function.Additional Fixtures:
mimic_2
intests/conftest.py
to provide test data for the new function.