Skip to content

Commit 5461b98

Browse files
glichtnerglichtner
committed
refactor: TemporalIndicatorCombination: Split into FixedWindow and PersonalWindow subclasses
- Introduce FixedWindowTemporalIndicatorCombination to handle fixed windows defined by interval_type or start/end times. - Introduce PersonalWindowTemporalIndicatorCombination to manage person-specific windows via interval_criterion. - Update operator and iteration logic to reduce conditional complexity. - Enhance documentation with detailed class-level docstrings. - Adjust factory methods to select the appropriate subclass based on provided parameters.
1 parent 7b41ba7 commit 5461b98

File tree

7 files changed

+444
-306
lines changed

7 files changed

+444
-306
lines changed

‎apps/graph/static/script.js

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -102,9 +102,18 @@ async function loadGraph(recommendationId) {
102102
return label;
103103

104104
} else if (ele.data('type').startsWith('Temporal') && ele.data('type').endsWith('Count')) {
105-
var label = ele.data('class')
105+
var label = ele.data('class');
106+
var intervalLabel = "";
107+
108+
if (ele.data('interval_type')) {
109+
intervalLabel = ele.data('interval_type');
110+
} else if (ele.data('start_time') && ele.data('end_time')) {
111+
intervalLabel = ele.data('start_time') + " - " + ele.data('end_time');
112+
} else if (ele.data('interval_criterion')) {
113+
intervalLabel = ele.data('interval_criterion');
114+
}
106115

107-
label += "[" + checkCount(ele) +'; ' + ele.data('interval_type') + "]";
116+
label += "[" + checkCount(ele) + "; " + intervalLabel + "]";
108117
return label;
109118
} else if (ele.data('type').endsWith('Count')) {
110119
return ele.data('class') + '[' + checkCount(ele) + ']';

‎execution_engine/converter/parser/fhir_parser_v1.py

Lines changed: 2 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
from typing import Callable, Union, cast
1+
from typing import Union, cast
22

33
from fhir.resources.evidencevariable import (
44
EvidenceVariable,
@@ -56,36 +56,6 @@ class FhirRecommendationParserV1(FhirRecommendationParserInterface):
5656
combination methods).
5757
"""
5858

59-
@staticmethod
60-
def _wrap_criteria_with_factory(
61-
combo: CriterionCombination,
62-
factory: Callable[
63-
[Criterion | CriterionCombination], TemporalIndicatorCombination
64-
],
65-
) -> CriterionCombination:
66-
"""
67-
Recursively wraps all Criterion instances within a combination using the factory.
68-
"""
69-
# Create a new combination of the same type with the same operator
70-
new_combo = combo.__class__(operator=combo.operator, category=combo.category)
71-
72-
# Loop through all elements
73-
for element in combo:
74-
if isinstance(element, LogicalCriterionCombination):
75-
# Recursively wrap nested combinations
76-
new_combo.add(
77-
FhirRecommendationParserV1._wrap_criteria_with_factory(
78-
element, factory
79-
)
80-
)
81-
elif isinstance(element, Criterion):
82-
# Wrap individual criteria with the factory
83-
new_combo.add(factory(element))
84-
else:
85-
raise ValueError(f"Unexpected element type: {type(element)}")
86-
87-
return new_combo
88-
8959
def parse_time_from_event(
9060
self,
9161
tfes: list[EvidenceVariableCharacteristicTimeFromEvent],
@@ -111,12 +81,7 @@ def parse_time_from_event(
11181

11282
converter = self.time_from_event_converters.get(tfe)
11383

114-
factory = converter.to_temporal_combination_factory()
115-
116-
new_combo = cast(
117-
self._wrap_criteria_with_factory(combo, factory),
118-
TemporalIndicatorCombination,
119-
)
84+
new_combo = converter.to_temporal_combination(combo)
12085

12186
return new_combo
12287

‎execution_engine/converter/time_from_event/abstract.py

Lines changed: 35 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -8,13 +8,40 @@
88
from execution_engine.fhir.util import get_coding
99
from execution_engine.omop.criterion.abstract import Criterion
1010
from execution_engine.omop.criterion.combination.combination import CriterionCombination
11+
from execution_engine.omop.criterion.combination.logical import (
12+
LogicalCriterionCombination,
13+
)
1114
from execution_engine.omop.criterion.combination.temporal import (
1215
TemporalIndicatorCombination,
1316
)
1417
from execution_engine.omop.vocabulary import AbstractVocabulary
1518
from execution_engine.util.value import ValueNumeric
1619

1720

21+
def _wrap_criteria_with_factory(
22+
combo: CriterionCombination,
23+
factory: Callable[[Criterion | CriterionCombination], TemporalIndicatorCombination],
24+
) -> CriterionCombination:
25+
"""
26+
Recursively wraps all Criterion instances within a combination using the factory.
27+
"""
28+
# Create a new combination of the same type with the same operator
29+
new_combo = combo.__class__(operator=combo.operator, category=combo.category)
30+
31+
# Loop through all elements
32+
for element in combo:
33+
if isinstance(element, LogicalCriterionCombination):
34+
# Recursively wrap nested combinations
35+
new_combo.add(_wrap_criteria_with_factory(element, factory))
36+
elif isinstance(element, Criterion):
37+
# Wrap individual criteria with the factory
38+
new_combo.add(factory(element))
39+
else:
40+
raise ValueError(f"Unexpected element type: {type(element)}")
41+
42+
return new_combo
43+
44+
1845
class TemporalIndicator(ABC):
1946
"""
2047
EvidenceVariable.characteristic.timeFromEvent in the context of CPG-on-EBM-on-FHIR.
@@ -35,11 +62,11 @@ def valid(cls, fhir: Element) -> bool:
3562
raise NotImplementedError("must be implemented by class")
3663

3764
@abstractmethod
38-
def to_temporal_combination_factory(
39-
self,
40-
) -> Callable[[Criterion | CriterionCombination], TemporalIndicatorCombination]:
65+
def to_temporal_combination(
66+
self, combo: Criterion | CriterionCombination
67+
) -> TemporalIndicatorCombination:
4168
"""
42-
Converts the TemporalIndicator to a TemporalIndicatorCombination.
69+
Wraps Criterion/CriterionCombinaion with a TemporalIndicatorCombination
4370
"""
4471
raise NotImplementedError("must be implemented by class")
4572

@@ -92,10 +119,10 @@ def valid(cls, fhir: Element) -> bool:
92119
return cls._event_vocabulary.is_system(cc.system) and cc.code == cls._event_code
93120

94121
@abstractmethod
95-
def to_temporal_combination_factory(
96-
self,
97-
) -> Callable[[Criterion | CriterionCombination], TemporalIndicatorCombination]:
122+
def to_temporal_combination(
123+
self, combo: Criterion | CriterionCombination
124+
) -> TemporalIndicatorCombination:
98125
"""
99-
Converts the TemporalIndicator to a TemporalIndicatorCombination or Criterion.
126+
Wraps Criterion/CriterionCombinaion with a TemporalIndicatorCombination
100127
"""
101128
raise NotImplementedError("must be implemented by class")

‎execution_engine/execution_graph/graph.py

Lines changed: 80 additions & 112 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,8 @@
1111
NonCommutativeLogicalCriterionCombination,
1212
)
1313
from execution_engine.omop.criterion.combination.temporal import (
14+
FixedWindowTemporalIndicatorCombination,
15+
PersonalWindowTemporalIndicatorCombination,
1416
TemporalIndicatorCombination,
1517
)
1618

@@ -226,6 +228,7 @@ def criterion_attr(attr: str) -> str | None:
226228
node_data["data"]["start_time"] = node.start_time
227229
node_data["data"]["end_time"] = node.end_time
228230
node_data["data"]["interval_type"] = node.interval_type
231+
node_data["data"]["interval_criterion"] = repr(node.interval_criterion)
229232

230233
if hasattr(node, "count_min"):
231234
node_data["data"]["count_min"] = node.count_min
@@ -387,136 +390,101 @@ def conjunction_from_combination(
387390
# The problem is that we need a non-criterion sink node of the intervention and population in order
388391
# to store the results to the database without the criterion_id (as the result of the whole
389392
# intervention or population of this population/intervention pair).
390-
assert (
393+
394+
if (
391395
comb.operator.operator
392-
== LogicalCriterionCombination.Operator.AND
393-
), (
394-
f"Invalid operator {str(comb.operator)} for root node. "
395-
f"Expected {LogicalCriterionCombination.Operator.AND}"
396-
)
396+
!= LogicalCriterionCombination.Operator.AND
397+
):
398+
raise AssertionError(
399+
f"Invalid operator {comb.operator} for root node. Expected AND."
400+
)
397401
return logic.NonSimplifiableAnd
398-
elif isinstance(comb, NonCommutativeLogicalCriterionCombination):
402+
403+
# Handle non-commutative combinations.
404+
if isinstance(comb, NonCommutativeLogicalCriterionCombination):
399405
return logic.ConditionalFilter
400-
elif comb.operator.operator == LogicalCriterionCombination.Operator.NOT:
401-
return logic.Not
402-
elif comb.operator.operator == LogicalCriterionCombination.Operator.AND:
403-
return logic.And
404-
elif comb.operator.operator == LogicalCriterionCombination.Operator.OR:
405-
return logic.Or
406-
elif (
407-
comb.operator.operator
408-
== LogicalCriterionCombination.Operator.ALL_OR_NONE
409-
):
410-
return logic.AllOrNone
411-
elif (
412-
comb.operator.operator
413-
== LogicalCriterionCombination.Operator.AT_LEAST
414-
):
415-
if comb.operator.threshold is None:
416-
raise ValueError(
417-
f"Threshold must be set for operator {comb.operator.operator}"
418-
)
419-
return lambda *args, category: logic.MinCount(
420-
*args, threshold=comb.operator.threshold, category=category # type: ignore
421-
)
422-
elif (
423-
comb.operator.operator
424-
== LogicalCriterionCombination.Operator.AT_MOST
425-
):
426-
if comb.operator.threshold is None:
427-
raise ValueError(
428-
f"Threshold must be set for operator {comb.operator.operator}"
429-
)
430-
return lambda *args, category: logic.MaxCount(
431-
*args, threshold=comb.operator.threshold, category=category # type: ignore
432-
)
433-
elif (
434-
comb.operator.operator
435-
== LogicalCriterionCombination.Operator.EXACTLY
436-
):
406+
407+
op = comb.operator.operator
408+
409+
# Mapping of simple logical operators.
410+
simple_ops = {
411+
LogicalCriterionCombination.Operator.NOT: logic.Not,
412+
LogicalCriterionCombination.Operator.AND: logic.And,
413+
LogicalCriterionCombination.Operator.OR: logic.Or,
414+
LogicalCriterionCombination.Operator.ALL_OR_NONE: logic.AllOrNone,
415+
}
416+
if op in simple_ops:
417+
return simple_ops[op]
418+
419+
# Mapping of count-based operators.
420+
count_ops = {
421+
LogicalCriterionCombination.Operator.AT_LEAST: logic.MinCount,
422+
LogicalCriterionCombination.Operator.AT_MOST: logic.MaxCount,
423+
LogicalCriterionCombination.Operator.EXACTLY: logic.ExactCount,
424+
}
425+
if op in count_ops:
437426
if comb.operator.threshold is None:
438427
raise ValueError(
439428
f"Threshold must be set for operator {comb.operator.operator}"
440429
)
441-
return lambda *args, category: logic.ExactCount(
442-
*args, threshold=comb.operator.threshold, category=category # type: ignore
443-
)
444-
else:
445-
raise NotImplementedError(
446-
f'Operator "{str(comb.operator)}" not implemented'
430+
return lambda *args, category: count_ops[op](
431+
*args, threshold=comb.operator.threshold, category=category
447432
)
448433

434+
raise NotImplementedError(f'Operator "{comb.operator}" not implemented')
435+
436+
###################################################################################
449437
elif isinstance(comb, TemporalIndicatorCombination):
450438

451-
tcomb: TemporalIndicatorCombination = comb
452-
interval_criterion: logic.Expr | logic.Symbol | None
439+
interval_criterion: logic.BaseExpr | None = None
440+
start_time = None
441+
end_time = None
442+
interval_type = None
453443

454-
if isinstance(tcomb.interval_criterion, CriterionCombination):
455-
interval_criterion = _traverse(tcomb.interval_criterion)
456-
elif isinstance(tcomb.interval_criterion, Criterion):
457-
interval_criterion = logic.Symbol(tcomb.interval_criterion)
458-
elif tcomb.interval_criterion is None:
459-
interval_criterion = None
460-
else:
461-
raise ValueError(
462-
f"Invalid interval criterion type: {type(tcomb.interval_criterion)}"
463-
)
444+
if isinstance(comb, PersonalWindowTemporalIndicatorCombination):
464445

465-
if (
466-
tcomb.operator.operator
467-
== TemporalIndicatorCombination.Operator.AT_LEAST
468-
):
469-
if tcomb.operator.threshold is None:
446+
if isinstance(comb.interval_criterion, CriterionCombination):
447+
interval_criterion = _traverse(comb.interval_criterion)
448+
elif isinstance(comb.interval_criterion, Criterion):
449+
interval_criterion = logic.Symbol(comb.interval_criterion)
450+
else:
470451
raise ValueError(
471-
f"Threshold must be set for operator {tcomb.operator.operator}"
452+
f"Invalid interval criterion type: {type(comb.interval_criterion)}"
472453
)
473-
return lambda *args, category: logic.TemporalMinCount(
474-
*args,
475-
threshold=tcomb.operator.threshold,
476-
category=category,
477-
start_time=tcomb.start_time,
478-
end_time=tcomb.end_time,
479-
interval_type=tcomb.interval_type, # type: ignore
480-
interval_criterion=interval_criterion,
481-
)
482-
elif (
483-
tcomb.operator.operator
484-
== TemporalIndicatorCombination.Operator.AT_MOST
485-
):
486-
if tcomb.operator.threshold is None:
487-
raise ValueError(
488-
f"Threshold must be set for operator {tcomb.operator.operator}"
489-
)
490-
return lambda *args, category: logic.TemporalMaxCount(
491-
*args,
492-
threshold=tcomb.operator.threshold,
493-
category=category, # type: ignore
494-
start_time=tcomb.start_time,
495-
end_time=tcomb.end_time,
496-
interval_type=tcomb.interval_type, # type: ignore
497-
interval_criterion=interval_criterion,
498-
)
499-
elif (
500-
tcomb.operator.operator
501-
== TemporalIndicatorCombination.Operator.EXACTLY
502-
):
503-
if tcomb.operator.threshold is None:
504-
raise ValueError(
505-
f"Threshold must be set for operator {tcomb.operator.operator}"
506-
)
507-
return lambda *args, category: logic.TemporalExactCount(
508-
*args,
509-
threshold=tcomb.operator.threshold,
510-
category=category,
511-
start_time=tcomb.start_time,
512-
end_time=tcomb.end_time,
513-
interval_type=tcomb.interval_type,
514-
interval_criterion=interval_criterion,
454+
455+
elif isinstance(comb, FixedWindowTemporalIndicatorCombination):
456+
start_time = comb.start_time
457+
end_time = comb.end_time
458+
interval_type = comb.interval_type
459+
460+
# Ensure a threshold is set.
461+
if comb.operator.threshold is None:
462+
raise ValueError(
463+
f"Threshold must be set for operator {comb.operator.operator}"
515464
)
516-
else:
465+
466+
# Map the operator to the corresponding logic function.
467+
op_map = {
468+
TemporalIndicatorCombination.Operator.AT_LEAST: logic.TemporalMinCount,
469+
TemporalIndicatorCombination.Operator.AT_MOST: logic.TemporalMaxCount,
470+
TemporalIndicatorCombination.Operator.EXACTLY: logic.TemporalExactCount,
471+
}
472+
op_func = op_map.get(comb.operator.operator, None)
473+
if op_func is None:
517474
raise NotImplementedError(
518-
f'Operator "{str(tcomb.operator)}" not implemented'
475+
f'Operator "{str(comb.operator)}" not implemented'
519476
)
477+
478+
return lambda *args, category: op_func(
479+
*args,
480+
threshold=comb.operator.threshold,
481+
category=category,
482+
start_time=start_time,
483+
end_time=end_time,
484+
interval_type=interval_type,
485+
interval_criterion=interval_criterion,
486+
)
487+
520488
else:
521489
raise ValueError(f"Invalid combination type: {type(comb)}")
522490

0 commit comments

Comments
 (0)