Enhancement/issue 822 #825
Conversation
|
Thanks a lot @aGuyLearning, awesome! Comments as per review :) |
ehrapy/tools/_sa.py
Outdated
| model = model_class() | ||
| model.fit(T, event_observed=E) | ||
| model.fit( |
There was a problem hiding this comment.
Add fit_left_censoring call upon argument input left
There was a problem hiding this comment.
check if nelson-aalen gracefully crashes with meaningful error message
| @@ -116,15 +116,10 @@ def glm( | |||
|
|
|||
|
|
|||
| def kmf( | |||
There was a problem hiding this comment.
one additional thing here; we'll also have to watch out for backwards compatibility.
Here, we could while still accepting the arguments raise a deprecation warning, where we briefly highlight the new behaviour.
- removed kwargs - updated documentation
|
On a sidenote: having a descriptive PR title is better here - the titles end up in the changelog, forgot to mention |
|
imblearn maintenance for sklearn usage (failing test) which @aGuyLearning noted is on the way btw |
| @@ -116,15 +117,19 @@ def glm( | |||
|
|
|||
|
|
|||
| def kmf( | |||
There was a problem hiding this comment.
Small trap here; None of the current arguments are keyword only, but could be called in the past with positionals.
If we make this API change of kmf(adata, duration_col, ….) , and not have all of the arguments being keyword only, then the previous style of calling kmf
`ep.tl.kmf(adata[:, ["mort_day_censored"]].X, adata[:, ["censor_flg”]].X)
will fail.
Instead of complex branching, I’m in favor of
- keeping kmf as deprecated function in a legacy module, working as it did in the past.
2.creating a new function kaplan_meier which has a) a name harmonized with nelson_aalen, weibull b) the same API structure c) is being recommended from the deprecation warning in 1.
There was a problem hiding this comment.
Here, interested in your take @Zethson. Does this seem like a reasonable option for you?
I think its the cleanest way of improving the API into the future
| @@ -97,7 +97,7 @@ def _sa_func_test(self, sa_function, sa_class, mimic_2_sa): | |||
|
|
|||
| def test_kmf(self, mimic_2_sa): | |||
| adata, _, _ = mimic_2_sa | |||
There was a problem hiding this comment.
If we'd make this change with the legacy module, could
- keep this test, and additionally check for the correct raise of a deprecation warning
- add a test for the new function
kaplan_meyer
| ci_labels=ci_labels, | ||
| weights=weights, | ||
| kmf = KaplanMeierFitter() | ||
| if censoring == "None" or "right": |
There was a problem hiding this comment.
this branching is not required anymore, but happens in _univariate_model if I understand correctly? might overlook something here though!
| @@ -386,12 +434,38 @@ def _univariate_model(adata: AnnData, duration_col: str, event_col: str, model_c | |||
| E = df[event_col] | |||
|
|
|||
| model = model_class() | |||
| model.fit(T, event_observed=E) | |||
| function_name = "fit" if censoring == "right" else "fit_left_censoring" | |||
There was a problem hiding this comment.
Should raise a ValueError here if its neither left nor right; as we don't pass the argument into a downstream function which would fail gracefully, it should fail here (else a user who e.g. enters censoring="rigth" gets accidentally an unexpected behaviour)
| adata: AnnData, | ||
| duration_col: str, | ||
| event_col: str, | ||
| timeline: list[float] | None = None, |
There was a problem hiding this comment.
In favor of taking this chance to make everything after event_col a keyword-only argument
| timeline: list[float] | None = None, | |
| *, | |
| timeline: list[float] | None = None, |
| def nelson_aalen( | ||
| adata: AnnData, | ||
| duration_col: str, | ||
| event_col: str, |
There was a problem hiding this comment.
Think event_col is optional also for NelsonAalen's fit right
| event_col: str, | |
| event_col: str | None = None, |
Description of changes
closes #822
Changed the method signature of univariate models to allow the user to pass parameters to the lifeline. Now weilbull, neil_aalen and kmf look like
adata: AnnData, duration_col: str, event_col: str, **kwargsand are passed on to the _univariate_model function
Technical details
this could be changed to explicitly have all parameters in each function and pass them on.
Additional context