Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify usage of teal_data for cdisc data format #336

Open
donyunardi opened this issue Oct 4, 2024 · 0 comments
Open

Simplify usage of teal_data for cdisc data format #336

donyunardi opened this issue Oct 4, 2024 · 0 comments
Labels

Comments

@donyunardi
Copy link
Contributor

donyunardi commented Oct 4, 2024

The more I think about this, the more I’m leaning towards deprecating cdisc_data.

The only purpose of this wrapper is to guess the join_keys for the datasets at creation. However, if we're promoting the use of within to avoid code repetition (which I completely agree), users will still need to set up the join_keys themselves, which defeats the purpose of the wrapper.

I think it’s much easier if users just work with teal_data. If they’re working with ADaM datasets, we can point them to the default_cdisc_join_keys helper object if they want to use it.

Originally posted by @donyunardi in insightsengineering/teal.modules.clinical#1230 (comment)

I have very similar thoughts. The concept of those two differs significantly. within() modifies the object in-place and require follow-up updates whereas cdisc_data() requires all the information on that object init. As we are moving towards the use of within() then I think that that our CDISC convenience function should follow the same paradigm to modify the object based on its current state. When doing that PR I got a little bit tired of setting keys and datanames separately. This feels repetetive and I think it can be easily wrapped up in a single convenience function - e.g. set_cdisc_attrs() or as_cdisc() so that the full processing can look like this: teal_data() |> within({...}) |> set_cdisc_attrs()

_Originally posted by @pawelru in
insightsengineering/teal.modules.clinical#1230 (comment)

Summary

We want to promote the use of the within function when building teal_data with the CDISC data format. As mentioned in the discussion above, the purpose of cdisc_data is lost when using within, since users will still need to set up join_keys manually.

Let's simplify this by having users only work with teal_data(). Any additional setup for CDISC data types can be handled afterward.

Note: We should also be able to avoid requiring users to set up datanames once we're finished with this.

@donyunardi donyunardi transferred this issue from insightsengineering/teal.modules.clinical Oct 4, 2024
@donyunardi donyunardi added the core label Oct 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant