From 82c5a1bfcfb22238de4daaf03e14d8c031a1bcf1 Mon Sep 17 00:00:00 2001 From: Samuel Johnson Date: Thu, 10 Sep 2026 14:09:10 -0400 Subject: [PATCH 01/13] tests and update --- .../data_services/base_data_service.py | 68 +++-- .../test_data_service/test_data_service.py | 246 ++++++++++++++++++ 2 files changed, 288 insertions(+), 26 deletions(-) diff --git a/cdisc_rules_engine/services/data_services/base_data_service.py b/cdisc_rules_engine/services/data_services/base_data_service.py index d1552d1aa..8efd8ad26 100644 --- a/cdisc_rules_engine/services/data_services/base_data_service.py +++ b/cdisc_rules_engine/services/data_services/base_data_service.py @@ -242,47 +242,63 @@ def handle_custom_domains( dataset: DatasetInterface, dataset_metadata: SDTMDatasetMetadata, ): - if self._contains_topic_variable(dataset, dataset_metadata.domain, "TERM"): + class_name = self._detect_class_from_topic_variable( + dataset, dataset_metadata.domain + ) + if class_name: + return class_name + if dataset_metadata.is_ap: + return self._get_associated_persons_inherit_class(dataset, dataset_metadata) + return None + + def _detect_class_from_topic_variable( + self, dataset: DatasetInterface, domain_key: str + ) -> Optional[str]: + if self._contains_topic_variable(dataset, domain_key, "TERM"): return EVENTS - if self._contains_topic_variable(dataset, dataset_metadata.domain, "TRT"): + if self._contains_topic_variable(dataset, domain_key, "TRT"): return INTERVENTIONS - if self._contains_topic_variable(dataset, dataset_metadata.domain, "QNAM"): + if self._contains_topic_variable(dataset, domain_key, "QNAM"): return RELATIONSHIP - if self._contains_topic_variable(dataset, dataset_metadata.domain, "TESTCD"): - if self._contains_topic_variable(dataset, dataset_metadata.domain, "OBJ"): + if self._contains_topic_variable(dataset, domain_key, "TESTCD"): + if self._contains_topic_variable(dataset, domain_key, "OBJ"): return FINDINGS_ABOUT return FINDINGS - if dataset_metadata.is_ap: - return self._get_associated_persons_inherit_class(dataset_metadata) return None def _get_associated_persons_inherit_class( self, + dataset: DatasetInterface, dataset_metadata: SDTMDatasetMetadata, - ): + ) -> Optional[str]: """ - Check with inherit class AP-- belongs to. + Check what inherit class an AP-- belongs to. + Tries, in order: + 1. The base domain (ap_suffix) as a physically submitted dataset, + inheriting its class via get_dataset_class. + 2. CDISC Library metadata, using ap_suffix as the domain key + 3. The AP dataset's own topic variable, keyed on ap_suffix (covers + sponsor-defined custom base domains that Library has no entry for) """ ap_suffix = dataset_metadata.ap_suffix - if not ap_suffix: - return None datasets = self.get_datasets() - if len(datasets) > 1: - new_dataset_metadata: SDTMDatasetMetadata = search_in_list( - datasets, lambda item: item.domain == ap_suffix - ) - if new_dataset_metadata: - if new_dataset_metadata.is_ap: - raise ValueError("Nested Associated Persons domain reference") - new_dataset = self.get_dataset(dataset_name=new_dataset_metadata.name) - else: - raise ValueError("Filename for domain doesn't exist") - return self.get_dataset_class( - new_dataset, - new_dataset_metadata, + new_dataset_metadata: SDTMDatasetMetadata = search_in_list( + datasets, lambda item: item.domain == ap_suffix + ) + if new_dataset_metadata: + new_dataset = self.get_dataset(dataset_name=new_dataset_metadata.name) + return self.get_dataset_class(new_dataset, new_dataset_metadata) + + if self.library_metadata: + class_data, _ = get_class_and_dataset_metadata( + self.library_metadata, + ap_suffix, ) - else: - return None + name = (class_data or {}).get("name") + if name: + return convert_library_class_name_to_ct_class(name) + + return self._detect_class_from_topic_variable(dataset, ap_suffix) def _contains_topic_variable( self, diff --git a/tests/unit/test_services/test_data_service/test_data_service.py b/tests/unit/test_services/test_data_service/test_data_service.py index b87677991..d12927fd8 100644 --- a/tests/unit/test_services/test_data_service/test_data_service.py +++ b/tests/unit/test_services/test_data_service/test_data_service.py @@ -452,3 +452,249 @@ def func(self, **kwargs): keys = [call[0][0] for call in cache_service.get.call_args_list] assert keys[0] != keys[1] + + +def test_get_associated_persons_inherit_class_base_domain_present(): + """Base domain (MH) is physically submitted alongside APMH: class should + be inherited via the real dataset, same path get_dataset_class already + exercises for APDM/DM in test_get_dataset_class_associated_domains.""" + datasets = [ + SDTMDatasetMetadata(**dataset) + for dataset in [ + { + "name": "APMH", + "first_record": {"DOMAIN": "APMH", "APID": "AP001"}, + "filename": "apmh.xpt", + }, + {"name": "MH", "first_record": {"DOMAIN": "MH"}, "filename": "mh.xpt"}, + ] + ] + ap_dataset = PandasDataset.from_dict({"DOMAIN": ["APMH"], "APID": ["test"]}) + mh_dataset = PandasDataset.from_dict({"DOMAIN": ["MH"], "MHTERM": ["test"]}) + path_to_dataset_map = {"APMH": ap_dataset, "MH": mh_dataset} + + with ( + patch( + "cdisc_rules_engine.services.data_services.LocalDataService.get_dataset", + side_effect=lambda dataset_name: path_to_dataset_map[dataset_name], + ), + patch( + "cdisc_rules_engine.services.data_services.LocalDataService.get_datasets", + return_value=datasets, + ), + ): + library_metadata: LibraryMetadataContainer = get_library_metadata_from_cache( + Validation_args( + f"{os.path.dirname(__file__)}/../../../../resources/cache", + 10, + [], + "", + "", + "sdtmig", + "3-4", + None, + None, + "", + "", + "", + False, + None, + None, + "", + "", + None, + "", + None, + False, + None, + None, + None, + None, + None, + None, + ) + ) + data_service = LocalDataService( + MagicMock(), + MagicMock(), + MagicMock(), + standard="sdtmig", + standard_version="3-4", + library_metadata=library_metadata, + ) + result = data_service._get_associated_persons_inherit_class( + ap_dataset, datasets[0] + ) + + assert result == EVENTS + + +def test_get_associated_persons_inherit_class_fallback_to_library(): + """Base domain (MH) is NOT submitted at all, and it's a real standard + domain: class should resolve from Library metadata via ap_suffix. + This is the fix for the 'Filename for domain doesn't exist' bug.""" + ap_metadata = SDTMDatasetMetadata( + name="APMH", + first_record={"DOMAIN": "APMH", "APID": "AP001"}, + filename="apmh.xpt", + ) + ap_dataset = PandasDataset.from_dict({"DOMAIN": ["APMH"], "APID": ["test"]}) + library_metadata: LibraryMetadataContainer = get_library_metadata_from_cache( + Validation_args( + f"{os.path.dirname(__file__)}/../../../../resources/cache", + 10, + [], + "", + "", + "sdtmig", + "3-4", + None, + None, + "", + "", + "", + False, + None, + None, + "", + "", + None, + "", + None, + False, + None, + None, + None, + None, + None, + None, + ) + ) + data_service = LocalDataService( + MagicMock(), + MagicMock(), + MagicMock(), + standard="sdtmig", + standard_version="3-4", + library_metadata=library_metadata, + ) + data_service.get_datasets = lambda: [ap_metadata] + + result = data_service._get_associated_persons_inherit_class(ap_dataset, ap_metadata) + + assert result == EVENTS + + +def test_get_associated_persons_inherit_class_custom_base_domain_resolves_via_topic_variable(): + """Base domain (ZZ) not submitted, and not a recognized Library domain + (sponsor-custom, e.g. APZZ on custom ZZ): resolves via the AP + dataset's own topic variable keyed on ap_suffix (ZZTERM).""" + dataset_metadata = SDTMDatasetMetadata( + name="APZZ", first_record={"DOMAIN": "APZZ", "APID": "AP001"} + ) + ap_dataset = PandasDataset.from_dict( + {"DOMAIN": ["APZZ"], "APID": ["test"], "ZZTERM": ["test"]} + ) + library_metadata: LibraryMetadataContainer = get_library_metadata_from_cache( + Validation_args( + f"{os.path.dirname(__file__)}/../../../../resources/cache", + 10, + [], + "", + "", + "sdtmig", + "3-4", + None, + None, + "", + "", + "", + False, + None, + None, + "", + "", + None, + "", + None, + False, + None, + None, + None, + None, + None, + None, + ) + ) + data_service = LocalDataService( + MagicMock(), + MagicMock(), + MagicMock(), + standard="sdtmig", + standard_version="3-4", + library_metadata=library_metadata, + ) + data_service.get_datasets = lambda: [dataset_metadata] + + result = data_service._get_associated_persons_inherit_class( + ap_dataset, dataset_metadata + ) + + assert result == EVENTS + + +def test_get_associated_persons_inherit_class_unresolvable_returns_none(): + """Base domain not submitted, no Library match, and no recognizable + topic variable: genuinely unclassifiable, returns None rather than + raise. This is the guard for the + convert_library_class_name_to_ct_class(None) bug.""" + dataset_metadata = SDTMDatasetMetadata( + name="APZZ", first_record={"DOMAIN": "APZZ", "APID": "AP001"} + ) + ap_dataset = PandasDataset.from_dict({"DOMAIN": ["APZZ"], "APID": ["test"]}) + library_metadata: LibraryMetadataContainer = get_library_metadata_from_cache( + Validation_args( + f"{os.path.dirname(__file__)}/../../../../resources/cache", + 10, + [], + "", + "", + "sdtmig", + "3-4", + None, + None, + "", + "", + "", + False, + None, + None, + "", + "", + None, + "", + None, + False, + None, + None, + None, + None, + None, + None, + ) + ) + data_service = LocalDataService( + MagicMock(), + MagicMock(), + MagicMock(), + standard="sdtmig", + standard_version="3-4", + library_metadata=library_metadata, + ) + data_service.get_datasets = lambda: [dataset_metadata] + + result = data_service._get_associated_persons_inherit_class( + ap_dataset, dataset_metadata + ) + + assert result is None From 4ec614aaed2bda490fc6d019695d72c1337cbc26 Mon Sep 17 00:00:00 2001 From: Samuel Johnson Date: Thu, 10 Sep 2026 14:10:42 -0400 Subject: [PATCH 02/13] tests --- .../test_data_service/test_data_service.py | 60 +------------------ 1 file changed, 1 insertion(+), 59 deletions(-) diff --git a/tests/unit/test_services/test_data_service/test_data_service.py b/tests/unit/test_services/test_data_service/test_data_service.py index d12927fd8..b86079bcb 100644 --- a/tests/unit/test_services/test_data_service/test_data_service.py +++ b/tests/unit/test_services/test_data_service/test_data_service.py @@ -531,8 +531,7 @@ def test_get_associated_persons_inherit_class_base_domain_present(): def test_get_associated_persons_inherit_class_fallback_to_library(): """Base domain (MH) is NOT submitted at all, and it's a real standard - domain: class should resolve from Library metadata via ap_suffix. - This is the fix for the 'Filename for domain doesn't exist' bug.""" + domain: class should resolve from Library metadata via ap_suffix.""" ap_metadata = SDTMDatasetMetadata( name="APMH", first_record={"DOMAIN": "APMH", "APID": "AP001"}, @@ -641,60 +640,3 @@ def test_get_associated_persons_inherit_class_custom_base_domain_resolves_via_to ) assert result == EVENTS - - -def test_get_associated_persons_inherit_class_unresolvable_returns_none(): - """Base domain not submitted, no Library match, and no recognizable - topic variable: genuinely unclassifiable, returns None rather than - raise. This is the guard for the - convert_library_class_name_to_ct_class(None) bug.""" - dataset_metadata = SDTMDatasetMetadata( - name="APZZ", first_record={"DOMAIN": "APZZ", "APID": "AP001"} - ) - ap_dataset = PandasDataset.from_dict({"DOMAIN": ["APZZ"], "APID": ["test"]}) - library_metadata: LibraryMetadataContainer = get_library_metadata_from_cache( - Validation_args( - f"{os.path.dirname(__file__)}/../../../../resources/cache", - 10, - [], - "", - "", - "sdtmig", - "3-4", - None, - None, - "", - "", - "", - False, - None, - None, - "", - "", - None, - "", - None, - False, - None, - None, - None, - None, - None, - None, - ) - ) - data_service = LocalDataService( - MagicMock(), - MagicMock(), - MagicMock(), - standard="sdtmig", - standard_version="3-4", - library_metadata=library_metadata, - ) - data_service.get_datasets = lambda: [dataset_metadata] - - result = data_service._get_associated_persons_inherit_class( - ap_dataset, dataset_metadata - ) - - assert result is None From 001562b17aaa034b9726cbe477f040259e024ede Mon Sep 17 00:00:00 2001 From: Samuel Johnson Date: Thu, 10 Sep 2026 16:21:05 -0400 Subject: [PATCH 03/13] removed some logic --- .../data_services/base_data_service.py | 18 ----- .../test_data_service/test_data_service.py | 80 ------------------- 2 files changed, 98 deletions(-) diff --git a/cdisc_rules_engine/services/data_services/base_data_service.py b/cdisc_rules_engine/services/data_services/base_data_service.py index 8efd8ad26..6f8a2c8de 100644 --- a/cdisc_rules_engine/services/data_services/base_data_service.py +++ b/cdisc_rules_engine/services/data_services/base_data_service.py @@ -31,7 +31,6 @@ from cdisc_rules_engine.services.data_readers import DataReaderFactory from cdisc_rules_engine.utilities.utils import ( get_dataset_cache_key_from_path, - search_in_list, replace_nan_values_in_df, ) from cdisc_rules_engine.utilities.sdtm_utilities import ( @@ -271,24 +270,7 @@ def _get_associated_persons_inherit_class( dataset: DatasetInterface, dataset_metadata: SDTMDatasetMetadata, ) -> Optional[str]: - """ - Check what inherit class an AP-- belongs to. - Tries, in order: - 1. The base domain (ap_suffix) as a physically submitted dataset, - inheriting its class via get_dataset_class. - 2. CDISC Library metadata, using ap_suffix as the domain key - 3. The AP dataset's own topic variable, keyed on ap_suffix (covers - sponsor-defined custom base domains that Library has no entry for) - """ ap_suffix = dataset_metadata.ap_suffix - datasets = self.get_datasets() - new_dataset_metadata: SDTMDatasetMetadata = search_in_list( - datasets, lambda item: item.domain == ap_suffix - ) - if new_dataset_metadata: - new_dataset = self.get_dataset(dataset_name=new_dataset_metadata.name) - return self.get_dataset_class(new_dataset, new_dataset_metadata) - if self.library_metadata: class_data, _ = get_class_and_dataset_metadata( self.library_metadata, diff --git a/tests/unit/test_services/test_data_service/test_data_service.py b/tests/unit/test_services/test_data_service/test_data_service.py index b86079bcb..9091e965f 100644 --- a/tests/unit/test_services/test_data_service/test_data_service.py +++ b/tests/unit/test_services/test_data_service/test_data_service.py @@ -454,84 +454,7 @@ def func(self, **kwargs): assert keys[0] != keys[1] -def test_get_associated_persons_inherit_class_base_domain_present(): - """Base domain (MH) is physically submitted alongside APMH: class should - be inherited via the real dataset, same path get_dataset_class already - exercises for APDM/DM in test_get_dataset_class_associated_domains.""" - datasets = [ - SDTMDatasetMetadata(**dataset) - for dataset in [ - { - "name": "APMH", - "first_record": {"DOMAIN": "APMH", "APID": "AP001"}, - "filename": "apmh.xpt", - }, - {"name": "MH", "first_record": {"DOMAIN": "MH"}, "filename": "mh.xpt"}, - ] - ] - ap_dataset = PandasDataset.from_dict({"DOMAIN": ["APMH"], "APID": ["test"]}) - mh_dataset = PandasDataset.from_dict({"DOMAIN": ["MH"], "MHTERM": ["test"]}) - path_to_dataset_map = {"APMH": ap_dataset, "MH": mh_dataset} - - with ( - patch( - "cdisc_rules_engine.services.data_services.LocalDataService.get_dataset", - side_effect=lambda dataset_name: path_to_dataset_map[dataset_name], - ), - patch( - "cdisc_rules_engine.services.data_services.LocalDataService.get_datasets", - return_value=datasets, - ), - ): - library_metadata: LibraryMetadataContainer = get_library_metadata_from_cache( - Validation_args( - f"{os.path.dirname(__file__)}/../../../../resources/cache", - 10, - [], - "", - "", - "sdtmig", - "3-4", - None, - None, - "", - "", - "", - False, - None, - None, - "", - "", - None, - "", - None, - False, - None, - None, - None, - None, - None, - None, - ) - ) - data_service = LocalDataService( - MagicMock(), - MagicMock(), - MagicMock(), - standard="sdtmig", - standard_version="3-4", - library_metadata=library_metadata, - ) - result = data_service._get_associated_persons_inherit_class( - ap_dataset, datasets[0] - ) - - assert result == EVENTS - - def test_get_associated_persons_inherit_class_fallback_to_library(): - """Base domain (MH) is NOT submitted at all, and it's a real standard - domain: class should resolve from Library metadata via ap_suffix.""" ap_metadata = SDTMDatasetMetadata( name="APMH", first_record={"DOMAIN": "APMH", "APID": "AP001"}, @@ -585,9 +508,6 @@ def test_get_associated_persons_inherit_class_fallback_to_library(): def test_get_associated_persons_inherit_class_custom_base_domain_resolves_via_topic_variable(): - """Base domain (ZZ) not submitted, and not a recognized Library domain - (sponsor-custom, e.g. APZZ on custom ZZ): resolves via the AP - dataset's own topic variable keyed on ap_suffix (ZZTERM).""" dataset_metadata = SDTMDatasetMetadata( name="APZZ", first_record={"DOMAIN": "APZZ", "APID": "AP001"} ) From fb28f8f16fa410e7c1bf274ab4256ff135083171 Mon Sep 17 00:00:00 2001 From: Samuel Johnson Date: Thu, 10 Sep 2026 16:44:12 -0400 Subject: [PATCH 04/13] ap_suffix for aprelsub and aprelspec --- cdisc_rules_engine/models/sdtm_dataset_metadata.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/cdisc_rules_engine/models/sdtm_dataset_metadata.py b/cdisc_rules_engine/models/sdtm_dataset_metadata.py index b8eec552e..a8013d3ad 100644 --- a/cdisc_rules_engine/models/sdtm_dataset_metadata.py +++ b/cdisc_rules_engine/models/sdtm_dataset_metadata.py @@ -32,6 +32,8 @@ class SDTMDatasetMetadata(DatasetMetadata): | APXX | APXX | False | APXX | XX | None | True | XX | False | XX | True | | SQAPXX | SQAPXX | True | None | | APXX | True | | False | XX | True | | FA | FA | False | FA | FA | None | False | | False | | | + | APRELSUB | APRELSUB | False | None | RELSUB | None | True | RELSUB | False | | | + | APRELSPEC | APRELSPEC | False | None | RELSPEC | None | True | RELSPEC | False | | | """ # noqa: E501 W291 @property @@ -90,6 +92,8 @@ def is_ap(self) -> bool: def ap_suffix(self) -> str: """ Returns the 2-character suffix (characters 3-4) from AP domains. + Falls back to the dataset name when there's no DOMAIN variable + (e.g. APRELSUB, APRELSPEC) Returns empty string if not an AP domain or for supp datasets. """ if not self.is_ap: @@ -98,4 +102,10 @@ def ap_suffix(self) -> str: return "" if isinstance(self.domain, str) and len(self.domain) >= 4: return self.domain[2::] + if ( + isinstance(self.name, str) + and len(self.name) >= 4 + and self.name.upper().startswith("AP") + ): + return self.name[2::] return "" From 2461c1bb47fdaae677c5fabc2607f3029ef61038 Mon Sep 17 00:00:00 2001 From: Samuel Johnson Date: Thu, 10 Sep 2026 17:10:19 -0400 Subject: [PATCH 05/13] tests --- .../test_operations/test_extract_metadata.py | 52 ++++++------------- 1 file changed, 17 insertions(+), 35 deletions(-) diff --git a/tests/unit/test_operations/test_extract_metadata.py b/tests/unit/test_operations/test_extract_metadata.py index 4bc5afe7c..e9bfefbdc 100644 --- a/tests/unit/test_operations/test_extract_metadata.py +++ b/tests/unit/test_operations/test_extract_metadata.py @@ -66,22 +66,20 @@ def _create_mock_service(dataset_name, first_record=None): @pytest.mark.parametrize( "dataset_name, first_record, expected_suffix", [ - ("APFA", None, ""), - ("APXX", None, ""), - ("APLB", None, ""), - ("", {"DOMAIN": "APFA", "APID": "AP001"}, "FA"), - ("AE", {"DOMAIN": "APFA", "APID": "AP001"}, "FA"), - ("AP", {"DOMAIN": "APFA", "APID": "AP001"}, "FA"), - ("APF", {"DOMAIN": "APFA", "APID": "AP001"}, "FA"), - ("AE", None, ""), - ("LB", None, ""), - ("AP", None, ""), - ("APF", None, ""), ("AE", {"DOMAIN": "AE"}, ""), - ("AE", {"DOMAIN": "LB"}, ""), - ("AE", {"DOMAIN": "AP"}, ""), + ("LB", {"DOMAIN": "LB"}, ""), + ("AE1", {"DOMAIN": "AE"}, ""), + ("APFA", {"DOMAIN": "APFA", "APID": "AP001"}, "FA"), + ("APLB", {"DOMAIN": "APLB", "APID": "AP001"}, "LB"), + ("APFA1", {"DOMAIN": "APFA", "APID": "AP001"}, "FA"), + ("APXX", None, "XX"), + ("APFA", {"APID": "AP001"}, "FA"), ("AE", {"DOMAIN": ""}, ""), ("AE", {"DOMAIN": None}, ""), + ("APRELSUB", {"APID": "AP001"}, "RELSUB"), + ("APRELSPEC", {"APID": "AP001"}, "RELSPEC"), + ("APRELSUB", None, "RELSUB"), + ("APRELSPEC", None, "RELSPEC"), ], ) @pytest.mark.parametrize("dataset_type", [PandasDataset, DaskDataset]) @@ -111,9 +109,12 @@ def test_extract_metadata_domain_suffix( ) result = operation.execute() assert operation_params.operation_id in result - assert all( - item == expected_suffix for item in result[operation_params.operation_id] - ) + actual_values = result[operation_params.operation_id] + for i, item in enumerate(actual_values): + assert item == expected_suffix, ( + f"row {i}: dataset_name={dataset_name!r} first_record={first_record!r} " + f"expected={expected_suffix!r} got={item!r}" + ) @pytest.mark.parametrize("dataset_type", [PandasDataset, DaskDataset]) @@ -135,22 +136,3 @@ def test_extract_metadata_domain_suffix_uses_domain( result = operation.execute() assert operation_params.operation_id in result assert all(item == "XX" for item in result[operation_params.operation_id]) - - -@pytest.mark.parametrize("dataset_type", [PandasDataset, DaskDataset]) -def test_extract_metadata_domain_suffix_empty_metadata( - operation_params: OperationParams, dataset_type -): - mock_data_service = _create_mock_service("APFA", None) - operation_params.dataframe = dataset_type.from_dict( - {"STUDYID": ["TEST_STUDY"], "DOMAIN": ["APFA"]} - ) - operation_params.dataframe_metadata = SDTMDatasetMetadata(name="APFA") - operation_params.target = "ap_suffix" - cache = InMemoryCacheService.get_instance() - operation = ExtractMetadata( - operation_params, operation_params.dataframe, cache, mock_data_service - ) - result = operation.execute() - assert operation_params.operation_id in result - assert all(item == "" for item in result[operation_params.operation_id]) From 328a60e1038bf0489f8d9163f289fca9cc412325 Mon Sep 17 00:00:00 2001 From: Samuel Johnson Date: Mon, 14 Sep 2026 16:51:53 -0400 Subject: [PATCH 06/13] custom logic --- cdisc_rules_engine/operations/domain_is_custom.py | 4 +++- cdisc_rules_engine/utilities/utils.py | 13 +++++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/cdisc_rules_engine/operations/domain_is_custom.py b/cdisc_rules_engine/operations/domain_is_custom.py index 641b3476d..7c34d887c 100644 --- a/cdisc_rules_engine/operations/domain_is_custom.py +++ b/cdisc_rules_engine/operations/domain_is_custom.py @@ -1,5 +1,6 @@ from cdisc_rules_engine.operations.base_operation import BaseOperation from cdisc_rules_engine.utilities.sdtm_utilities import is_custom_domain +from cdisc_rules_engine.utilities.utils import get_dataset_standard_check_name class DomainIsCustom(BaseOperation): @@ -9,4 +10,5 @@ def _execute_operation(self): given domain is in standard domains. If no -> the domain is custom. """ - return is_custom_domain(self.library_metadata, self.params.domain) + check_name = get_dataset_standard_check_name(self.params.dataframe_metadata) + return is_custom_domain(self.library_metadata, check_name) diff --git a/cdisc_rules_engine/utilities/utils.py b/cdisc_rules_engine/utilities/utils.py index 40fe038d7..3749096cb 100644 --- a/cdisc_rules_engine/utilities/utils.py +++ b/cdisc_rules_engine/utilities/utils.py @@ -19,6 +19,7 @@ from cdisc_rules_engine.models.base_validation_entity import BaseValidationEntity from cdisc_rules_engine.check_operators.helpers import is_valid_date from cdisc_rules_engine.constants.adam_products import ADAM_PRODUCTS +from cdisc_rules_engine.models.sdtm_dataset_metadata import SDTMDatasetMetadata def convert_dataclass_to_superclass[T](instance: object, superclass: type[T]) -> T: @@ -414,3 +415,15 @@ def custom_str_conversion(x): elif isinstance(x, float): return f"{x:.0f}" if x.is_integer() else str(x).strip() return x + + +def get_dataset_standard_check_name( + dataset_metadata: Optional[SDTMDatasetMetadata], +) -> Optional[str]: + if dataset_metadata is None: + return None + if dataset_metadata.is_supp: + return "SUPPQUAL" + if dataset_metadata.is_ap: + return dataset_metadata.ap_suffix or None + return dataset_metadata.domain or dataset_metadata.name From f43798c8d6bcdf7dba94a0f98f8265d48adcd752 Mon Sep 17 00:00:00 2001 From: Samuel Johnson Date: Tue, 15 Sep 2026 15:28:45 -0400 Subject: [PATCH 07/13] update schema and tests --- resources/schema/rule/CORE-base.json | 2 + .../test_operations/test_domain_is_custom.py | 73 +++++++++---------- 2 files changed, 35 insertions(+), 40 deletions(-) diff --git a/resources/schema/rule/CORE-base.json b/resources/schema/rule/CORE-base.json index 16dc33f8d..753bab577 100644 --- a/resources/schema/rule/CORE-base.json +++ b/resources/schema/rule/CORE-base.json @@ -74,12 +74,14 @@ "ALL", "AP--", "APRELSUB", + "APRELSPEC", "POOLDEF", "RELREC", "RELREF", "RELSPEC", "RELSUB", "SUPP--", + "SQ--", "TUMOR" ], "type": "string" diff --git a/tests/unit/test_operations/test_domain_is_custom.py b/tests/unit/test_operations/test_domain_is_custom.py index d1caf924c..111a70219 100644 --- a/tests/unit/test_operations/test_domain_is_custom.py +++ b/tests/unit/test_operations/test_domain_is_custom.py @@ -5,6 +5,7 @@ from cdisc_rules_engine.models.library_metadata_container import ( LibraryMetadataContainer, ) +from cdisc_rules_engine.models.sdtm_dataset_metadata import SDTMDatasetMetadata import pytest from cdisc_rules_engine.models.operation_params import OperationParams from cdisc_rules_engine.operations.domain_is_custom import DomainIsCustom @@ -13,89 +14,81 @@ @pytest.mark.parametrize( - "dataframe, domain, standard, standard_version, expected", + "dataframe, domain, dataframe_metadata, standard, standard_version, expected", [ ( PandasDataset.from_dict( - { - "STUDYID": [ - "TEST_STUDY", - "TEST_STUDY", - "TEST_STUDY", - ], - "AETERM": [ - "test", - "test", - "test", - ], - } + {"STUDYID": ["TEST_STUDY"] * 3, "AETERM": ["test"] * 3} ), "AE", + SDTMDatasetMetadata(name="AE", first_record={"DOMAIN": "AE"}), "sdtmig", "3-4", False, ), ( DaskDataset.from_dict( - { - "STUDYID": [ - "TEST_STUDY", - "TEST_STUDY", - "TEST_STUDY", - ], - "AETERM": [ - "test", - "test", - "test", - ], - } + {"STUDYID": ["TEST_STUDY"] * 3, "AETERM": ["test"] * 3} ), "AE", + SDTMDatasetMetadata(name="AE", first_record={"DOMAIN": "AE"}), "sdtmig", "3-4", False, ), ( PandasDataset.from_dict( - { - "STUDYID": [ - "TEST_STUDY", - "TEST_STUDY", - "TEST_STUDY", - ], - "BCTERM": [ - "test", - "test", - "test", - ], - } + {"STUDYID": ["TEST_STUDY"] * 3, "BCTERM": ["test"] * 3} ), "BC", + SDTMDatasetMetadata(name="BC", first_record={"DOMAIN": "BC"}), "sdtmig", "3-4", True, ), + ( + PandasDataset.from_dict( + {"STUDYID": ["TEST_STUDY"] * 3, "APID": ["AP001"] * 3} + ), + "APMH", + SDTMDatasetMetadata( + name="APMH", first_record={"DOMAIN": "APMH", "APID": "AP001"} + ), + "sdtmig-ap", + "1-0", + False, + ), + ( + PandasDataset.from_dict( + {"STUDYID": ["TEST_STUDY"] * 3, "RDOMAIN": ["AE"] * 3} + ), + "SUPPAE", + SDTMDatasetMetadata(name="SUPPAE", first_record={"RDOMAIN": "AE"}), + "sdtmig", + "3-4", + False, + ), ], ) def test_domain_is_custom( operation_params: OperationParams, dataframe: DatasetInterface, domain: str, + dataframe_metadata: SDTMDatasetMetadata, standard: str, standard_version: str, expected: bool, ): standard_metadata = { - "dataset_names": {"AE"}, + "dataset_names": {"AE", "MH", "SUPPQUAL"}, } operation_params.dataframe = dataframe operation_params.domain = domain + operation_params.dataframe_metadata = dataframe_metadata operation_params.standard = standard operation_params.standard_version = standard_version - # save model metadata to cache cache = InMemoryCacheService.get_instance() library_metadata = LibraryMetadataContainer(standard_metadata=standard_metadata) - # execute operation data_service = LocalDataService.get_instance( cache_service=cache, config=ConfigService() ) From 22333e0667f8eb2dd30c4897137a90048081a25f Mon Sep 17 00:00:00 2001 From: github-actions Date: Tue, 15 Sep 2026 19:30:12 +0000 Subject: [PATCH 08/13] Update merged schema files with markdown descriptions --- resources/schema/rule-merged/CORE-base.json | 2 ++ 1 file changed, 2 insertions(+) diff --git a/resources/schema/rule-merged/CORE-base.json b/resources/schema/rule-merged/CORE-base.json index c7ce4f5e8..74eee86ad 100644 --- a/resources/schema/rule-merged/CORE-base.json +++ b/resources/schema/rule-merged/CORE-base.json @@ -74,12 +74,14 @@ "ALL", "AP--", "APRELSUB", + "APRELSPEC", "POOLDEF", "RELREC", "RELREF", "RELSPEC", "RELSUB", "SUPP--", + "SQ--", "TUMOR" ], "type": "string" From 16c33e103a2b1affea8f0a9b59eca3c0bec7005d Mon Sep 17 00:00:00 2001 From: Samuel Johnson Date: Wed, 16 Sep 2026 18:00:12 -0400 Subject: [PATCH 09/13] split logic --- cdisc_rules_engine/constants/domains.py | 3 - .../constants/rule_constants.py | 1 + .../models/sdtm_dataset_metadata.py | 52 +++-- .../utilities/rule_processor.py | 103 +++------- resources/schema/rule-merged/CORE-base.json | 7 +- resources/schema/rule/CORE-base.json | 7 +- .../test_utilities/test_rule_processor.py | 179 +++++++----------- 7 files changed, 118 insertions(+), 234 deletions(-) diff --git a/cdisc_rules_engine/constants/domains.py b/cdisc_rules_engine/constants/domains.py index 166b3598f..9e74f4cb9 100644 --- a/cdisc_rules_engine/constants/domains.py +++ b/cdisc_rules_engine/constants/domains.py @@ -1,4 +1 @@ SUPPLEMENTARY_DOMAINS = ("SUPP", "SQ") -AP_DOMAIN: str = "AP" -APFA_DOMAIN: str = "APFA" -APRELSUB_DOMAIN: str = "APRELSUB" diff --git a/cdisc_rules_engine/constants/rule_constants.py b/cdisc_rules_engine/constants/rule_constants.py index a0b35dac1..6d18dae11 100644 --- a/cdisc_rules_engine/constants/rule_constants.py +++ b/cdisc_rules_engine/constants/rule_constants.py @@ -1 +1,2 @@ ALL_KEYWORD: str = "ALL" +SPLIT_DATASETS_KEYWORD = "SPLIT DATASETS" diff --git a/cdisc_rules_engine/models/sdtm_dataset_metadata.py b/cdisc_rules_engine/models/sdtm_dataset_metadata.py index a8013d3ad..36a7ca800 100644 --- a/cdisc_rules_engine/models/sdtm_dataset_metadata.py +++ b/cdisc_rules_engine/models/sdtm_dataset_metadata.py @@ -12,28 +12,28 @@ class SDTMDatasetMetadata(DatasetMetadata): """ Examples - | name | unsplit_name | is_supp | domain | wildcard_replacement | rdomain | is_ap | ap_suffix | domain_is_custom | related_domain | related_domain_is_custom | - | -------- | ------------ | ------- | ------ | -------------------- | ------- | ----- | --------- | ---------------- | -------------- | ------------------------ | - | QS | QS | False | QS | QS | None | False | | False | | | - | QSX | QS | False | QS | QS | None | False | | False | | | - | QSXX | QS | False | QS | QS | None | False | | False | | | - | SUPPQS | SUPPQS | True | None | | QS | False | | False | QS | | - | SUPPQSX | SUPPQS | True | None | | QS | False | | False | QS | | - | SUPPQSXX | SUPPQS | True | None | | QS | False | | False | QS | | - | APQS | APQS | False | APQS | QS | None | True | QS | False | QS | | - | APQSX | APQS | False | APQS | QS | None | True | QS | False | QS | | - | APQSXX | APQS | False | APQS | QS | None | True | QS | False | QS | | - | SQAPQS | SQAPQS | True | None | | APQS | True | | False | QS | | - | SQAPQSX | SQAPQS | True | None | | APQS | True | | False | QS | | - | SQAPQSXX | SQAPQS | True | None | | APQS | True | | False | | | - | RELREC | RELREC | False | None | | None | False | | False | | | - | XX | XX | False | XX | XX | None | False | | True | | | - | SUPPXX | SUPPXX | True | None | | XX | False | | False | XX | True | - | APXX | APXX | False | APXX | XX | None | True | XX | False | XX | True | - | SQAPXX | SQAPXX | True | None | | APXX | True | | False | XX | True | - | FA | FA | False | FA | FA | None | False | | False | | | - | APRELSUB | APRELSUB | False | None | RELSUB | None | True | RELSUB | False | | | - | APRELSPEC | APRELSPEC | False | None | RELSPEC | None | True | RELSPEC | False | | | +| name | unsplit_name | is_supp | is_split | domain | wildcard_replacement | rdomain | is_ap | ap_suffix | domain_is_custom | related_domain | related_domain_is_custom | +| --------- | ------------ | ------- | -------- | ------ | --------------------- | ------- | ----- | --------- | ---------------- | --------------- | ------------------------- | +| QS | QS | False | False | QS | QS | None | False | | False | | | +| QSX | QS | False | True | QS | QS | None | False | | False | | | +| QSXX | QS | False | True | QS | QS | None | False | | False | | | +| SUPPQS | SUPPQS | True | False | | | QS | False | | False | QS | | +| SUPPQSX | SUPPQS | True | True | | | QS | False | | False | QS | | +| SUPPQSXX | SUPPQS | True | True | | | QS | False | | False | QS | | +| APQS | APQS | False | False | QS | QS | None | True | QS | False | QS | | +| APQSX | APQS | False | True | QS | QS | None | True | QS | False | QS | | +| APQSXX | APQS | False | True | QS | QS | None | True | QS | False | QS | | +| SQAPQS | SQAPQS | True | False | | | APQS | False | | False | QS | | +| SQAPQSX | SQAPQS | True | True | | | APQS | False | | False | QS | | +| SQAPQSXX | SQAPQS | True | True | | | APQS | False | | False | | | +| RELREC | RELREC | False | False | | | None | False | | False | | | +| XX | XX | False | False | XX | XX | None | False | | True | | | +| SUPPXX | SUPPXX | True | False | | | XX | False | | False | XX | True | +| APXX | APXX | False | False | XX | XX | None | True | XX | False | XX | True | +| SQAPXX | SQAPXX | True | False | | | APXX | False | | False | XX | True | +| FA | FA | False | False | FA | FA | None | False | | False | | | +| APRELSUB | APRELSUB | False | False | | RELSUB | None | True | RELSUB | False | | | +| APRELSPEC | APRELSPEC | False | False | | RELSPEC | None | True | RELSPEC | False | | | """ # noqa: E501 W291 @property @@ -74,18 +74,12 @@ def is_ap(self) -> bool: """ Returns true if APID variable exists in first_record for non-supp datasets, or if the dataset name starts with AP for non-supp datasets, - or if rdomain starts with AP for supp datasets. """ if self.is_supp: - return ( - isinstance(self.rdomain, str) - and len(self.rdomain) == 4 - and self.rdomain.startswith("AP") - ) + return False first_record = self.first_record or {} if "APID" in first_record: return True - return self.name.startswith("AP") @property diff --git a/cdisc_rules_engine/utilities/rule_processor.py b/cdisc_rules_engine/utilities/rule_processor.py index dce7ec69c..0ff63c7ad 100644 --- a/cdisc_rules_engine/utilities/rule_processor.py +++ b/cdisc_rules_engine/utilities/rule_processor.py @@ -18,11 +18,6 @@ FINDINGS_ABOUT, FINDINGS, ) -from cdisc_rules_engine.constants.domains import ( - AP_DOMAIN, - APFA_DOMAIN, - SUPPLEMENTARY_DOMAINS, -) from cdisc_rules_engine.constants.rule_constants import ALL_KEYWORD from cdisc_rules_engine.constants.use_cases import USE_CASE_DOMAINS from cdisc_rules_engine.interfaces import ConditionInterface @@ -60,32 +55,35 @@ def __init__( self.cache = cache self.library_metadata = library_metadata + DATASET_PROPERTY_KEYWORDS: dict = { + "SUPPQUAL": lambda md: bool(md.is_supp), + "AP--": lambda md: bool(md.is_ap), + "SPLIT DATASETS": lambda md: bool(md.is_split), + "AP SPLIT DATASETS": lambda md: ( + bool(md.is_ap) and bool(md.is_split) and not bool(md.is_supp) + ), + } + + @classmethod + def _matched_property_keyword( + cls, dataset_metadata: SDTMDatasetMetadata, domains_to_check: List[str] + ) -> bool: + for keyword, predicate in cls.DATASET_PROPERTY_KEYWORDS.items(): + if keyword in domains_to_check and predicate(dataset_metadata): + return True + return False + @classmethod def rule_applies_to_domain( cls, dataset_metadata: SDTMDatasetMetadata, rule: dict ) -> bool: - """ - Check that rule is applicable to dataset domain - """ domains = rule.get("domains") or {} - include_split_datasets: bool = domains.get("include_split_datasets") - included_domains = domains.get("Include", []) excluded_domains = domains.get("Exclude", []) - is_included = cls._is_domain_name_included( - dataset_metadata, included_domains, include_split_datasets - ) + is_included = cls._is_domain_name_included(dataset_metadata, included_domains) is_excluded = cls._is_domain_name_excluded(dataset_metadata, excluded_domains) - # additional check for split domains based on the flag - is_excluded, is_included = cls._handle_split_domains( - dataset_metadata.is_split, - include_split_datasets, - is_excluded, - is_included, - ) - return is_included and not is_excluded @classmethod @@ -93,22 +91,8 @@ def _is_domain_name_included( cls, dataset_metadata: SDTMDatasetMetadata, included_domains: List[str], - include_split_datasets: bool, ) -> bool: - """ - If included domains aren't specified - and include_split_datasets is True, - and it is not a split dataset - -> domain is not included - If included domains are specified, - and the domain is not in the list of included domains, - and domain doesn't match with AP / APFA / APRELSUB / SUPP / SQ naming pattern - -> domain is not included. - In other cases domain is included - """ if not included_domains: - if include_split_datasets is True and not dataset_metadata.is_split: - return False return True if ( @@ -117,7 +101,7 @@ def _is_domain_name_included( or ALL_KEYWORD in included_domains ): return True - if cls._domain_matched_ap_or_supp(dataset_metadata, included_domains): + if cls._matched_property_keyword(dataset_metadata, included_domains): return True return False @@ -128,7 +112,6 @@ def _is_domain_name_excluded( """ If excluded domains are specified, and the domain is in the list of excluded domains, - or domain name match with AP / APFA / APRELSUB / SUPP / SQ naming pattern domain is excluded. In other cases domain is not excluded. @@ -143,49 +126,7 @@ def _is_domain_name_excluded( or ALL_KEYWORD in excluded_domains ): return True - if cls._domain_matched_ap_or_supp(dataset_metadata, excluded_domains): - return True - return False - - @classmethod - def _handle_split_domains( - cls, - is_split_domain: bool, - include_split_datasets: bool, - is_excluded: bool, - is_included: bool, - ) -> Tuple[bool, bool]: - """ - HANDLING SPLIT DOMAINS - - If include_split_datasets is True - - add split domains to the list of included domains. - If no included domains specified, only validate split domains - - If include_split_datasets is False - Exclude split domains - If include_split_datasets is None - Do nothing - """ - if include_split_datasets is True and is_split_domain and not is_excluded: - is_included = True - if include_split_datasets is False and is_split_domain: - is_excluded = True - return is_excluded, is_included - - @classmethod - def _domain_matched_ap_or_supp( - cls, dataset_metadata: SDTMDatasetMetadata, domains_to_check: List[str] - ) -> bool: - """ - Check that domain name match with only - AP / APFA / APRELSUB / SUPP / SQ naming pattern - """ - domains_to_check = set(domains_to_check) - supp_domains = {f"{domain}--" for domain in SUPPLEMENTARY_DOMAINS} - ap_domains = {f"{AP_DOMAIN}--", f"{APFA_DOMAIN}--"} - - if dataset_metadata.is_supp and (domains_to_check & supp_domains): - return True - if dataset_metadata.is_ap and (domains_to_check & ap_domains): + if cls._matched_property_keyword(dataset_metadata, excluded_domains): return True return False @@ -214,7 +155,7 @@ def rule_applies_to_data_structure( def rule_applies_to_class( self, rule, - dataset_metadata: SDTMDatasetMetadata, + dataset_metadata: "SDTMDatasetMetadata", ): """ If included classes are specified and the class diff --git a/resources/schema/rule-merged/CORE-base.json b/resources/schema/rule-merged/CORE-base.json index 74eee86ad..1c8938837 100644 --- a/resources/schema/rule-merged/CORE-base.json +++ b/resources/schema/rule-merged/CORE-base.json @@ -80,9 +80,10 @@ "RELREF", "RELSPEC", "RELSUB", - "SUPP--", - "SQ--", - "TUMOR" + "SUPPQUAL", + "TUMOR", + "SPLIT DATASETS", + "AP SPLIT DATASETS" ], "type": "string" }, diff --git a/resources/schema/rule/CORE-base.json b/resources/schema/rule/CORE-base.json index 753bab577..f012883d4 100644 --- a/resources/schema/rule/CORE-base.json +++ b/resources/schema/rule/CORE-base.json @@ -80,9 +80,10 @@ "RELREF", "RELSPEC", "RELSUB", - "SUPP--", - "SQ--", - "TUMOR" + "SUPPQUAL", + "TUMOR", + "SPLIT DATASETS", + "AP SPLIT DATASETS" ], "type": "string" }, diff --git a/tests/unit/test_utilities/test_rule_processor.py b/tests/unit/test_utilities/test_rule_processor.py index 273e8606c..e6aa88ecc 100644 --- a/tests/unit/test_utilities/test_rule_processor.py +++ b/tests/unit/test_utilities/test_rule_processor.py @@ -27,20 +27,18 @@ @pytest.mark.parametrize( "name, rule_metadata, outcome", [ - ("SQAE", {"domains": {"Exclude": ["SUPP--"]}}, False), - ("SQAE", {"domains": {"Exclude": ["SUPP--", "SQ--"]}}, False), - ("SQAE", {"domains": {"Include": ["SQ--"]}}, True), + ("SQAE", {"domains": {"Exclude": ["SUPPQUAL"]}}, False), + ("SQAE", {"domains": {"Include": ["SUPPQUAL"]}}, True), ("SQAE", {"domains": {"Exclude": [ALL_KEYWORD]}}, False), - ("SQAE", {"domains": {"Include": ["SUPP--"]}}, True), ("SQAE", {"domains": {"Include": [ALL_KEYWORD]}}, True), ("AE", {"domains": {"Include": ["AE"]}}, True), ("AE", {"domains": {"Include": [ALL_KEYWORD]}}, True), ("AE", {"domains": {"Exclude": ["AE"]}}, False), ("AE", {"domains": {"Exclude": [ALL_KEYWORD]}}, False), ("AE", {"domains": {"Include": ["TV"]}}, False), - ("SUPPAE", {"domains": {"Exclude": ["SUPP--"]}}, False), + ("SUPPAE", {"domains": {"Exclude": ["SUPPQUAL"]}}, False), ("SUPPAE", {"domains": {"Exclude": [ALL_KEYWORD]}}, False), - ("SUPPAE", {"domains": {"Include": ["SUPP--"]}}, True), + ("SUPPAE", {"domains": {"Include": ["SUPPQUAL"]}}, True), ("SUPPAE", {"domains": {"Include": [ALL_KEYWORD]}}, True), ("APTE", {"domains": {"Exclude": ["AP--"]}}, False), ("APTE", {"domains": {"Exclude": [ALL_KEYWORD]}}, False), @@ -50,9 +48,9 @@ ("APRELSUB", {"domains": {"Exclude": [ALL_KEYWORD]}}, False), ("APRELSUB", {"domains": {"Include": ["APRELSUB"]}}, True), ("APRELSUB", {"domains": {"Include": [ALL_KEYWORD]}}, True), - ("APFASU", {"domains": {"Exclude": ["APFA--"]}}, False), + ("APFASU", {"domains": {"Exclude": ["AP--"]}}, False), ("APFASU", {"domains": {"Exclude": [ALL_KEYWORD]}}, False), - ("APFASU", {"domains": {"Include": ["APFA--"]}}, True), + ("APFASU", {"domains": {"Include": ["AP--"]}}, True), ("APFASU", {"domains": {"Include": [ALL_KEYWORD]}}, True), ], ) @@ -73,133 +71,84 @@ def test_rule_applies_to_domain(mock_data_service, name, rule_metadata, outcome) "rule_domains, expected_results", [ ( - { - "Include": [ALL_KEYWORD], - "include_split_datasets": True, # Includes all - }, - [True, True, True, True, True, True], + {"Include": ["AP SPLIT DATASETS"]}, + [True, True, False, False], ), ( - {}, # Domains are included by default + {"Include": ["SPLIT DATASETS"]}, + [True, True, True, False], + ), + ( + {"Exclude": ["AP SPLIT DATASETS"]}, + [False, False, True, True], + ), + ( + {"Include": ["SPLIT DATASETS"], "Exclude": ["AP SPLIT DATASETS"]}, + [False, False, True, False], + ), + ], +) +def test_rule_applies_to_domain_ap_split_datasets( + mock_data_service, rule_domains: dict, expected_results: List[bool] +): + rule = {"domains": rule_domains} + datasets: List[dict] = [ + {"name": "APFA1", "domain": "APFA"}, # AP + split + {"name": "APFA2", "domain": "APFA"}, # AP + split + {"name": "QS1", "domain": "QS"}, # split, but not AP + {"name": "AE", "domain": "AE"}, # neither split nor AP + ] + processor = RuleProcessor(mock_data_service, InMemoryCacheService()) + results = [ + processor.rule_applies_to_domain( + SDTMDatasetMetadata( + name=dataset["name"], first_record={"DOMAIN": dataset["domain"]} + ), + rule, + ) + for dataset in datasets + ] + assert results == expected_results + + +@pytest.mark.parametrize( + "rule_domains, expected_results", + [ + ( + {"Include": [ALL_KEYWORD]}, [True, True, True, True, True, True], ), ( - { - "include_split_datasets": True, # Only include split datasets - }, - [ - False, - False, - True, - True, - True, - True, - ], + {}, # Domains are included by default + [True, True, True, True, True, True], ), ( - { - "Include": ["AE"], - "include_split_datasets": True, - }, - [ - True, - False, - True, - True, - True, - True, - ], + {"Include": ["SPLIT DATASETS"]}, # Only split datasets + [False, False, True, True, True, True], ), ( - # Only run on split datasets except SUPP - { - "Exclude": ["SUPP--"], - "include_split_datasets": True, - }, - [ - False, - False, - True, - True, - False, - False, - ], + {"Include": ["AE", "SPLIT DATASETS"]}, + [True, False, True, True, True, True], ), ( - { - "Include": [ - "EC", - ], - "Exclude": ["SUPP--"], - "include_split_datasets": True, - }, - [ - False, - True, - True, - True, - False, - False, - ], + {"Include": ["SPLIT DATASETS"], "Exclude": ["SUPPQUAL"]}, + [False, False, True, True, False, False], ), ( - { - "Include": [ - "EC", - "QS", - ], - "include_split_datasets": False, - }, - [ - False, - True, - False, - False, - False, - False, - ], + {"Include": ["EC", "SPLIT DATASETS"], "Exclude": ["SUPPQUAL"]}, + [False, True, True, True, False, False], ), ( - { - "Include": [ - "EC", - "QS", - ], - "include_split_datasets": True, - }, - [ - False, - True, - True, - True, - True, - True, - ], + {"Include": ["EC", "QS"]}, + [False, True, True, True, False, False], ), ( {"Include": ["QS"]}, - [ - False, - False, - True, - True, - False, - False, - ], + [False, False, True, True, False, False], ), ( - { - "Exclude": ["QS", "SUPPQS"], - "include_split_datasets": True, - }, - [ - False, - False, - False, - False, - False, - False, - ], + {"Exclude": ["SPLIT DATASETS"]}, + [True, True, False, False, False, False], ), ( {"Include": ["EC"]}, From 0c2a1bb9db5c8aa72cf5b0541a01761724ae6fa2 Mon Sep 17 00:00:00 2001 From: Samuel Johnson Date: Wed, 16 Sep 2026 18:27:02 -0400 Subject: [PATCH 10/13] tests --- tests/resources/CoreIssue576/Rule_underscores.json | 2 +- tests/unit/test_utilities/test_sdtm_utils.py | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/resources/CoreIssue576/Rule_underscores.json b/tests/resources/CoreIssue576/Rule_underscores.json index 4a8bd5c7b..6e575a6a1 100644 --- a/tests/resources/CoreIssue576/Rule_underscores.json +++ b/tests/resources/CoreIssue576/Rule_underscores.json @@ -84,7 +84,7 @@ }, "Domains": { "Include": [ - "SUPP--" + "SUPPQUAL" ] } }, diff --git a/tests/unit/test_utilities/test_sdtm_utils.py b/tests/unit/test_utilities/test_sdtm_utils.py index aee50aa8b..2e2c30f0b 100644 --- a/tests/unit/test_utilities/test_sdtm_utils.py +++ b/tests/unit/test_utilities/test_sdtm_utils.py @@ -281,9 +281,9 @@ def test_is_supp_dataset(mock_dataset, expected): ({"first_record": None}, False), ({"first_record": {}}, False), ({}, False), - ({"name": "SQAPQS", "first_record": {"RDOMAIN": "APQS"}}, True), - ({"name": "SQAPQSX", "first_record": {"RDOMAIN": "APQS"}}, True), - ({"name": "SQAPQSXX", "first_record": {"RDOMAIN": "APQS"}}, True), + ({"name": "SQAPQS", "first_record": {"RDOMAIN": "APQS"}}, False), + ({"name": "SQAPQSX", "first_record": {"RDOMAIN": "APQS"}}, False), + ({"name": "SQAPQSXX", "first_record": {"RDOMAIN": "APQS"}}, False), ({"name": "SUPPQS", "first_record": {"RDOMAIN": "QS"}}, False), ({"name": "SQAPQS", "first_record": {"RDOMAIN": "AP"}}, False), ({"name": "SQAPQS", "first_record": {"RDOMAIN": "APF"}}, False), From c59f3656e1794042bb21591051a6479f6123ea4c Mon Sep 17 00:00:00 2001 From: Samuel Johnson Date: Thu, 1 Oct 2026 09:53:16 -0400 Subject: [PATCH 11/13] update rule scope --- tests/resources/CoreIssue1699/rule.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/resources/CoreIssue1699/rule.yml b/tests/resources/CoreIssue1699/rule.yml index 8af4f6ff7..5f99f79bf 100644 --- a/tests/resources/CoreIssue1699/rule.yml +++ b/tests/resources/CoreIssue1699/rule.yml @@ -176,5 +176,5 @@ Scope: - RELATIONSHIP Domains: Include: - - SUPP-- + - SUPPQUAL Sensitivity: Record From d67b1b2047e9bf9bf0a8008a741df1aed6618afc Mon Sep 17 00:00:00 2001 From: Samuel Johnson Date: Thu, 8 Oct 2026 12:41:54 -0400 Subject: [PATCH 12/13] suppqual rollback --- cdisc_rules_engine/utilities/rule_processor.py | 3 ++- resources/schema/rule-merged/CORE-base.json | 3 ++- resources/schema/rule/CORE-base.json | 3 ++- tests/unit/test_utilities/test_rule_processor.py | 16 ++++++++++------ 4 files changed, 16 insertions(+), 9 deletions(-) diff --git a/cdisc_rules_engine/utilities/rule_processor.py b/cdisc_rules_engine/utilities/rule_processor.py index 0ff63c7ad..3c97bdb59 100644 --- a/cdisc_rules_engine/utilities/rule_processor.py +++ b/cdisc_rules_engine/utilities/rule_processor.py @@ -56,7 +56,8 @@ def __init__( self.library_metadata = library_metadata DATASET_PROPERTY_KEYWORDS: dict = { - "SUPPQUAL": lambda md: bool(md.is_supp), + "SUPP--": lambda md: bool(md.is_supp), + "SQ--": lambda md: bool(md.is_supp), "AP--": lambda md: bool(md.is_ap), "SPLIT DATASETS": lambda md: bool(md.is_split), "AP SPLIT DATASETS": lambda md: ( diff --git a/resources/schema/rule-merged/CORE-base.json b/resources/schema/rule-merged/CORE-base.json index 1c8938837..6bf8ede12 100644 --- a/resources/schema/rule-merged/CORE-base.json +++ b/resources/schema/rule-merged/CORE-base.json @@ -80,7 +80,8 @@ "RELREF", "RELSPEC", "RELSUB", - "SUPPQUAL", + "SQ--", + "SUPP--", "TUMOR", "SPLIT DATASETS", "AP SPLIT DATASETS" diff --git a/resources/schema/rule/CORE-base.json b/resources/schema/rule/CORE-base.json index f012883d4..8b1b647f3 100644 --- a/resources/schema/rule/CORE-base.json +++ b/resources/schema/rule/CORE-base.json @@ -80,7 +80,8 @@ "RELREF", "RELSPEC", "RELSUB", - "SUPPQUAL", + "SQ--", + "SUPP--", "TUMOR", "SPLIT DATASETS", "AP SPLIT DATASETS" diff --git a/tests/unit/test_utilities/test_rule_processor.py b/tests/unit/test_utilities/test_rule_processor.py index e6aa88ecc..7509e7aa6 100644 --- a/tests/unit/test_utilities/test_rule_processor.py +++ b/tests/unit/test_utilities/test_rule_processor.py @@ -27,8 +27,10 @@ @pytest.mark.parametrize( "name, rule_metadata, outcome", [ - ("SQAE", {"domains": {"Exclude": ["SUPPQUAL"]}}, False), - ("SQAE", {"domains": {"Include": ["SUPPQUAL"]}}, True), + ("SQAE", {"domains": {"Exclude": ["SUPP--"]}}, False), + ("SQAE", {"domains": {"Exclude": ["SQ--"]}}, False), + ("SQAE", {"domains": {"Include": ["SQ--"]}}, True), + ("SQAE", {"domains": {"Include": ["SUPP--"]}}, True), ("SQAE", {"domains": {"Exclude": [ALL_KEYWORD]}}, False), ("SQAE", {"domains": {"Include": [ALL_KEYWORD]}}, True), ("AE", {"domains": {"Include": ["AE"]}}, True), @@ -36,9 +38,11 @@ ("AE", {"domains": {"Exclude": ["AE"]}}, False), ("AE", {"domains": {"Exclude": [ALL_KEYWORD]}}, False), ("AE", {"domains": {"Include": ["TV"]}}, False), - ("SUPPAE", {"domains": {"Exclude": ["SUPPQUAL"]}}, False), + ("SUPPAE", {"domains": {"Exclude": ["SUPP--"]}}, False), + ("SUPPAE", {"domains": {"Exclude": ["SQ--"]}}, False), + ("SUPPAE", {"domains": {"Include": ["SUPP--"]}}, True), + ("SUPPAE", {"domains": {"Include": ["SQ--"]}}, True), ("SUPPAE", {"domains": {"Exclude": [ALL_KEYWORD]}}, False), - ("SUPPAE", {"domains": {"Include": ["SUPPQUAL"]}}, True), ("SUPPAE", {"domains": {"Include": [ALL_KEYWORD]}}, True), ("APTE", {"domains": {"Exclude": ["AP--"]}}, False), ("APTE", {"domains": {"Exclude": [ALL_KEYWORD]}}, False), @@ -131,11 +135,11 @@ def test_rule_applies_to_domain_ap_split_datasets( [True, False, True, True, True, True], ), ( - {"Include": ["SPLIT DATASETS"], "Exclude": ["SUPPQUAL"]}, + {"Include": ["SPLIT DATASETS"], "Exclude": ["SUPP--"]}, [False, False, True, True, False, False], ), ( - {"Include": ["EC", "SPLIT DATASETS"], "Exclude": ["SUPPQUAL"]}, + {"Include": ["EC", "SPLIT DATASETS"], "Exclude": ["SUPP--"]}, [False, True, True, True, False, False], ), ( From 50863ed80060aeab017436b4fda5cb7238490d77 Mon Sep 17 00:00:00 2001 From: Samuel Johnson Date: Fri, 9 Oct 2026 08:54:34 -0400 Subject: [PATCH 13/13] regression --- tests/resources/CoreIssue1699/rule.yml | 2 +- tests/resources/CoreIssue576/Rule_underscores.json | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/resources/CoreIssue1699/rule.yml b/tests/resources/CoreIssue1699/rule.yml index 5f99f79bf..8af4f6ff7 100644 --- a/tests/resources/CoreIssue1699/rule.yml +++ b/tests/resources/CoreIssue1699/rule.yml @@ -176,5 +176,5 @@ Scope: - RELATIONSHIP Domains: Include: - - SUPPQUAL + - SUPP-- Sensitivity: Record diff --git a/tests/resources/CoreIssue576/Rule_underscores.json b/tests/resources/CoreIssue576/Rule_underscores.json index 6e575a6a1..4a8bd5c7b 100644 --- a/tests/resources/CoreIssue576/Rule_underscores.json +++ b/tests/resources/CoreIssue576/Rule_underscores.json @@ -84,7 +84,7 @@ }, "Domains": { "Include": [ - "SUPPQUAL" + "SUPP--" ] } },