#1197 Fix domain_is_custom operation for TIG standards and refactor domain custom check logic#1511
Conversation
…custom check logic
| domain_name = dataset.get("name") | ||
| if domain_name: | ||
| domain_names.add(domain_name) | ||
| for dataset in standard_data.get("datasets", []): |
There was a problem hiding this comment.
Could you please explain briefly why we are now dataset names too and adding them to domain names?
There was a problem hiding this comment.
I noticed that TIG standards can have datasets in two places: nested under classes (like SDTMIG/SENDIG) and at the top level of the standard_data structure. The first loop handles datasets in classes. The second loop captures top-level datasets that appear in TIG standards.
I saw this pattern in _get_therapeutic_ig_codelists (around line 528), where TIG standards with substandard 'sdtm' or 'send' process top-level datasets separately from class datasets. Without the second loop, we'd miss domain names for those top-level datasets in TIG standards, which would cause domain_is_custom to incorrectly classify them as custom domains.
There was a problem hiding this comment.
Thank you for your explanation.
There was a problem hiding this comment.
for the future @RakeshBobba03 model defines the datasets and standard defines the domains. Your code conflated the two.
Figured it was worth pointing out--it took me awhile to get model and standard down in my mind
There was a problem hiding this comment.
I have reviewed and done manual validation. All seems good. Before merging could you please add a case where you use DASK too for new added test cases.
RamilCDISC
left a comment
RamilCDISC
left a comment
There was a problem hiding this comment.
The PR fixes the domain_is_custom operation for TIG standards by ensuring all domain names are properly included. The validation was done by:
- Reviewing the PR for any unwanted code or comments.
- Reviewing the PR logic in accordance with AC.
- Ensuring all unit and regression testing pass.
- Ensuring all relevant testing is updated.
- Ensuring testing covers PANDAS and DASK both dataset implementation.
- Running manual validations on CLI and rule editor.
- Running manual validation using the dataset and rule for which the issue was reported.
- Running manual validation for SDTMIG dataset.
- Running manual validation for TIG with different substandard.
|
Hey @RakeshBobba03 @RamilCDISC I was looking this over and it seems this is not completely correct. The YP domain in the test data Els provided is custom and the PMVALU column for it is expected to be flagged as an issue. Currently it is skipping it and saying it is out of scope for the rule. I have not reopened the branch but I moved the ticket out of the closed column as it is not closed. |
4 participants
Issue #1197: This PR implements custom domain detection by introducing a LibraryMetadataContainer that stores standard metadata (including domain sets) and exposes an is_domain_custom method to determine if a domain exists in the standard, a new domain_is_custom operation for rule authors to check custom domains, updates to CDISCLibraryService to extract and cache domain names from standard metadata via get_standard_details, and integration in RuleProcessor to use custom domain detection for use case validation, ensuring custom domains are handled correctly when pulling variable metadata from the model hierarchy rather than the implementation guide.