1332: correctly substitute AP domains placeholders #1468
Conversation
| def unsplit_name(self) -> str: | ||
| if self.domain: | ||
| return self.domain | ||
| return self.domain_cleaned |
There was a problem hiding this comment.
I am not certain about this replacement. With AP-- there are spots where we would want the full AP-- domain to be present and those tend to be when unsplit name is called.
perform_rule_operations getting called with unsplit name also leads to some introduced bugs with this change-- the exception handling around operation execution reports the domain-- this would point us to EC and not ADEC.
I do think the other replacements make sense--when we are replacing wildcards we want to use what is after AP for column names. Otherwise we want to keep AP-- domain intact for IDing that dataset for results/library lookup
There was a problem hiding this comment.
Ok, rolling back to just using self.domain here
| domain_details = search_in_list_of_dicts( | ||
| datasets, | ||
| lambda item: domain == (item.domain or item.name), | ||
| lambda item: domain == (item.domain_cleaned or item.name), |
There was a problem hiding this comment.
AP handling within the sdtm utility will need to be changed a fair bit so I would remove this.
we will need a way to detect AP domains.
Associated person is defined in the standard
and we need the variables from such.
the model does not define associated persons domain but the other variables in the dataset come from the domain the associated person is being attached to EG for APEG, RELSUP for APRELSUP, etc.
There was a problem hiding this comment.
@eljanssens I am not terribly familiar with AP domains but is the above correct. I strayed a bit from the wildcard replacement for AP but we will need to address the AP domains not being correctly fetched in library as well
There was a problem hiding this comment.
@SFJohnson24 AP is not a well known standard but as far as I understand it: SDTM model for AP defines which variables are unique for AP domains and then indeed the rest of the variables are a copy of the normal SDTM IG domain. Required and unique for AP domains are for example APID. If there is APID, RSUBJID, RDEVID or SREL present, it is an AP domain. The other variables are exactly the same as the normal domain so for EG for example: DOMAIN = EG, EGTESTCD, EGCAT,... Does that help?
There was a problem hiding this comment.
worth noting there is now an is_ap property https://github.com/cdisc-org/cdisc-rules-engine/pull/1439/files
…into 1332-correctly-substitue-placeholders-for-ap-domains
…https://github.com/cdisc-org/cdisc-rules-engine into 1332-correctly-substitue-placeholders-for-ap-domains
|
@alexfurmenkov looks like some unit tests are failing |
…into 1332-correctly-substitue-placeholders-for-ap-domains
| ({"first_record": {"DOMAIN": "APQS", "APID": "AP003"}}, "QS"), | ||
| ({"first_record": {"DOMAIN": "APLB", "APID": "AP004"}}, "LB"), | ||
| ({"first_record": {"DOMAIN": "APFAMH", "APID": "AP005"}}, "FA"), | ||
| ({"first_record": {"DOMAIN": "APFAMH", "APID": "AP005"}}, "FAMH"), |
There was a problem hiding this comment.
@SFJohnson24 I had to adjust this unit test because of our change in sdtm_dataset_metadata.py. Is this case checking a real domain that starts with AP and has a suffix that consists of 2 letters only? Or do we consider everything that comes after AP as a suffix?
There was a problem hiding this comment.
@alexfurmenkov i was not sure of the answer but looked it up and I do believe you can make an AP-- dataset for a split domain like FACE or FAMH. The domain on that test dataset would be APFA, not APFAMH so technically that test data first record data is invalid
SFJohnson24
left a comment
SFJohnson24
left a comment
There was a problem hiding this comment.
This PR correctly adds logic to remove the AP prefix when trying to replace wildcards in dataset variable's prefixes. Of note, this ticket does not make cg00013 executable for AP domains but this is a step towards that. #1479 needs to be done to correctly grab the library metadata for CG00013 execution
SFJohnson24
deleted the
1332-correctly-substitue-placeholders-for-ap-domains
branch
No description provided.