cdisc-org / cdisc-rules-engine

Open source offering of the cdisc rules engine
MIT License
49 stars 12 forks source link

Enhancement request: extend is_ordered_set and is_not_ordered_set for accepting arrays (lists) #172

Open JozefAerts opened 2 years ago

JozefAerts commented 2 years ago

The current implementation of is_ordered_set and is_not_ordered_set only accepts a single parameter value. This is fine for e.g. checking the order of --SEQ within USUBJID. It does however not allow to e.g. check SEND rule 130 (https://jira.cdisc.org/browse/CORERULES-577): "The values of SESTDTC provide the chronological order of the actual subject Elements. SESEQ should be assigned to be consistent with the chronological order." Or otherwise said: within each subject, SESEQ must be ordered by value of SESTDTC.

I think (but am not 100% sure) that overloading the method to also accept arrays would enable the above.

nhaydel commented 2 years ago

Does the target_is_sorted_by operator not solve this issue: https://github.com/cdisc-org/business-rules/blob/master/business_rules/operators.py#L1054?

JozefAerts commented 2 years ago

I am stilly trying to figure out how "target_is_sorted_by" works. I have tried a few things for rule SENDIG-130, but whatever I try, I get an exception "string indices must be integers". Here is an example snapshot of my code:

snapshot

gerrycampion commented 2 years ago

A few things:

JozefAerts commented 2 years ago

Thanks Gerry! I uploaded test files: https://cdisc.sharepoint.com/sites/CORERules/Shared%20Documents/Forms/AllItems.aspx?id=%2Fsites%2FCORERules%2FShared%20Documents%2FunitTesting%2FSENDIG%2FSENDIG130%2Fpositive%2F01%2Fdata&viewid=ecd009a9%2Db73b%2D415b%2D9e41%2Df9d04a69edf4 https://cdisc.sharepoint.com/sites/CORERules/Shared%20Documents/Forms/AllItems.aspx?id=%2Fsites%2FCORERules%2FShared%20Documents%2FunitTesting%2FSENDIG%2FSENDIG130%2Fnegative%2F01%2Fdata&viewid=ecd009a9%2Db73b%2D415b%2D9e41%2Df9d04a69edf4 and adapted the YAML to use "within", but keep getting the same "string indices must be integers" exception. Many thanks! Jozef

nhaydel commented 2 years ago

@gcampion-cdisc is_ordered_by is meant to determine whether or not a target is sorted in ascending or descending order. target_is_sorted_by is meant to check that within certain groups, the target is sorted in ascending or descending order. There is that slight within difference that adds some complexity, hence the two operators. Though I don't see us using is_ordered_by that often

gerrycampion commented 2 years ago

@nhaydel within aside, is it correct to say that is_ordered_by looks at the order (position) of records in a dataset whereas target_is_sorted_by looks at the order of values in a variable relative to the order of values in other variables?

If that is the case, I do see these as distinct operations. If that is not the case, and the only difference is the use of within, then I think these operations should be combined.

nhaydel commented 2 years ago

@gcampion-cdisc that is the case. target_is_sorted_by looks at the order of values in a variable relative to the order of values in another variable. I will provide an example in the next day or so