Feat/oneinpatient twooutpatient phenotype #171
Conversation
| relative_time_range, | ||
| categorical_filter_inpatient, | ||
| categorical_filter_outpatient, | ||
| return_date, |
There was a problem hiding this comment.
What values of return_date are supported?
There was a problem hiding this comment.
@elinadimi , please look at other Phenotypes to see how we are using type hinting!
|
Hi Ellie!! Congratulations on your first PR! It looks great, I just have some comments for you to think about. I will leave final approval to Alex because I think he is deeper in this topic. |
sprivite
added
the
Major additions
| relative_time_range, | ||
| categorical_filter_inpatient, | ||
| categorical_filter_outpatient, | ||
| return_date, |
There was a problem hiding this comment.
@elinadimi , please look at other Phenotypes to see how we are using type hinting!
| ) | ||
|
|
||
| pt_final = LogicPhenotype( | ||
| name=name, expression=pt_inpatient | pt_outpatient_two_occurrences |
There was a problem hiding this comment.
you are not using the return_date keyword argument. please add this argument to the final LogicPhenotype
| - ISTHMajorBleed: api/phenotypes/factory/isth_major_bleed.md | ||
| - StackableRegimen: api/phenotypes/factory/stackable_regimen.md | ||
| - one_inpatient_two_outpatient: api/phenotypes/factory/one_inpatient_two_outpatient.md | ||
|
|
There was a problem hiding this comment.
could you delete this white space? (there's one empty line)
| """ | ||
|
|
||
|
|
||
| # EventCountPhenotype only filters when given a value_filter or relative_time_range. |
There was a problem hiding this comment.
could you move this comment to line 127? it refers to the eventcountphenotype, not the codelistphenotype here. also please add the # TODO expose the EventCountPhenotype's value_filter and relative_time_range keyword arguments to the user of OneInpatientTwoOutpatientPhenotype as two well named keyword arguments
There was a problem hiding this comment.
also, the phenex library comments are not hard wrapped; i.e. we don't have to manually do wrapping.
| ) -> LogicPhenotype: | ||
| """ | ||
| OneInpatientTwoOutpatientPhenotype identifies patients who meet a combined rule: | ||
| **at least one inpatient event OR at least two outpatient events** within a specified |
There was a problem hiding this comment.
please do soft wrapping
| joined_table = joined_table.mutate(BOOLEAN=ibis.null().cast("boolean")) | ||
|
|
||
| return joined_table | ||
| return joined_table.drop_duplicates() |
| name: str, | ||
| domain: str, | ||
| codelist: Codelist, | ||
| relative_time_range: Union[RelativeTimeRangeFilter, List[RelativeTimeRangeFilter]], |
There was a problem hiding this comment.
please add the default value of 'none' to the relative_time_range! then it can be used as an entry_criterion
| pt_outpatient_two_occurrences = EventCountPhenotype( | ||
| phenotype=pt_outpatient, | ||
| value_filter=ValueFilter(min_value=GreaterThanOrEqualTo(2)), | ||
| relative_time_range=RelativeTimeRangeFilter(), |
There was a problem hiding this comment.
please do create a new keyword argument called 'outpatient_relative_time_range' that is passed to the pt_outpatient_two_occurrences. if it is None, then create a RelativeTimeRangeFilter(), else pass the user defined outpatient_relative_time_range!!! then user can specify the number of days between outpatient events.
Introducing one inpatient two outpatient phenotype