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

Make JoinKeys related changes due to refactor #92

Merged
merged 73 commits into from
Nov 20, 2023
Merged

Conversation

gogonzo
Copy link
Contributor

@gogonzo gogonzo commented Nov 8, 2023

Open over #75 to implement improved JoinKeys API

Also closes #95

Fixed apps:

  • basic-teal
  • exploratory
  • safety
  • efficacy
  • patient-profile
  • early-dev
  • longitudinal
  • RNA-seq
  • python

Full installation

staged.dependencies::dependency_table(
  project = "insightsengineering/teal.data@https://github.com",
  project_type = "repo@host",
  ref = "591-migrate-to-teal_data@78_simplify_joinkeys@main",
  verbose = 1
) |> staged.dependencies::install_deps(install_direction = "all")

efficacy/app.R Outdated Show resolved Hide resolved
exploratory/app.R Outdated Show resolved Hide resolved
patient-profile/app.R Outdated Show resolved Hide resolved
safety/app.R Outdated Show resolved Hide resolved
@m7pr
Copy link
Contributor

m7pr commented Nov 8, 2023

Hey, overall looks good and simplifies the code redundancy. I noticed few places where we can substitute data@join_keys with join_keys(data)

@vedhav vedhav changed the base branch from main to dev November 8, 2023 14:35
@vedhav
Copy link
Contributor

vedhav commented Nov 8, 2023

Have switched the target to dev because for now, we can only have successful deployments with multiple branches. Soon, we plan to see if there is something better we can do about this.

@vedhav
Copy link
Contributor

vedhav commented Nov 9, 2023

After the teal_data_module is created in the teal PR #957 and the new python_dataset_connector is created the python app will be fixed.

datanames <- c("ADSL", "ADRS", "ADLB", "ADLBPCA")
datanames(data) <- datanames

join_keys(data) <- default_cdisc_join_keys[datanames]
Copy link
Contributor

@kartikeyakirar kartikeyakirar Nov 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we exclude ADSLPCA since it is not present in the default_cdisc_join_keys. Instead, we should simply use c("ADSL", "ADRS", "ADLB").to enhance clarity in the subsequent step ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to default_cdisc_join_keys[c("ADSL", "ADRS", "ADLB")] # no ADLBPCA defaults

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's just a matter of DRY here. We already store the datanames in a variable so it's convenient to do a subset like this. Now it's okay, previously we used to get a join key set for ADLB with NULL.

datanames(data) <- datanames

# set join keys
join_keys(data) <- default_cdisc_join_keys[datanames] # get default keys by name
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@averissimo @gogonzo what about datanames not present in default_cdisc_join_keys ?

> join_keys(data)
A join_keys object containing foreign keys between 9 datasets:
ADSL: [STUDYID, USUBJID]
  <-- ADAE: [STUDYID, USUBJID]
  <-- ADCM: [STUDYID, USUBJID]
  <-- ADEX: [STUDYID, USUBJID]
  <-- ADLB: [STUDYID, USUBJID]
  <-- ADRS: [STUDYID, USUBJID]
  <-> ADTR: [STUDYID, USUBJID]
  <-> ADTRWF: [STUDYID, USUBJID]
  <-> ADRSSWIM: [STUDYID, USUBJID]
ADAE: [STUDYID, USUBJID, ASTDTM, AETERM, AESEQ]
  --> ADSL: [STUDYID, USUBJID]
  --* (implicit via parent with): ADCM, ADEX, ADLB, ADRS
ADCM: [STUDYID, USUBJID, ASTDTM, CMSEQ, ATC1CD, ATC2CD, ATC3CD, ATC4CD]
  --> ADSL: [STUDYID, USUBJID]
  --* (implicit via parent with): ADAE, ADEX, ADLB, ADRS
ADEX: [STUDYID, USUBJID, PARCAT1, PARAMCD, AVISITN, ASTDTM, EXSEQ]
  --> ADSL: [STUDYID, USUBJID]
  --* (implicit via parent with): ADAE, ADCM, ADLB, ADRS
ADLB: [STUDYID, USUBJID, PARAMCD, AVISIT]
  --> ADSL: [STUDYID, USUBJID]
  --* (implicit via parent with): ADAE, ADCM, ADEX, ADRS
ADRS: [STUDYID, USUBJID, PARAMCD, AVISIT]
  --> ADSL: [STUDYID, USUBJID]
  --* (implicit via parent with): ADAE, ADCM, ADEX, ADLB
ADTR: [STUDYID, USUBJID, PARAMCD, AVISIT]
  <-> ADSL: [STUDYID, USUBJID]
ADTRWF: [STUDYID, USUBJID, PARAMCD, AVISIT]
  <-> ADSL: [STUDYID, USUBJID]
ADRSSWIM: [STUDYID, USUBJID, PARAMCD, AVISIT]
  <-> ADSL: [STUDYID, USUBJID] 
> old_key$get_join_keys()
A join_keys object containing foreign keys between 9 datasets:
ADSL: [STUDYID, USUBJID]
  <-- ADAE: [STUDYID, USUBJID]
  <-- ADEX: [STUDYID, USUBJID]
  <-- ADCM: [STUDYID, USUBJID]
  <-- ADTR: [STUDYID, USUBJID]
  <-- ADTRWF: [STUDYID, USUBJID]
  <-- ADRS: [STUDYID, USUBJID]
  <-- ADRSSWIM: [STUDYID, USUBJID]
  <-- ADLB: [STUDYID, USUBJID]
ADAE: [STUDYID, USUBJID, AETERM, AESEQ]
  --> ADSL: [STUDYID, USUBJID]
  <-> ADEX: [STUDYID, USUBJID]
  <-> ADCM: [STUDYID, USUBJID]
  <-> ADTR: [STUDYID, USUBJID]
  <-> ADTRWF: [STUDYID, USUBJID]
  <-> ADRS: [STUDYID, USUBJID]
  <-> ADRSSWIM: [STUDYID, USUBJID]
  <-> ADLB: [STUDYID, USUBJID]
ADEX: [STUDYID, USUBJID, PARCAT1, PARAMCD, AVISITN, ASTDTM, EXSEQ]
  --> ADSL: [STUDYID, USUBJID]
  <-> ADAE: [STUDYID, USUBJID]
  <-> ADCM: [STUDYID, USUBJID]
  <-> ADTR: [STUDYID, USUBJID]
  <-> ADTRWF: [STUDYID, USUBJID]
  <-> ADRS: [STUDYID, USUBJID]
  <-> ADRSSWIM: [STUDYID, USUBJID]
  <-> ADLB: [STUDYID, USUBJID]
ADCM: [STUDYID, USUBJID, ASTDTM, CMSEQ, ATC1CD, ATC2CD, ATC3CD, ATC4CD]
  --> ADSL: [STUDYID, USUBJID]
  <-> ADAE: [STUDYID, USUBJID]
  <-> ADEX: [STUDYID, USUBJID]
  <-> ADTR: [STUDYID, USUBJID]
  <-> ADTRWF: [STUDYID, USUBJID]
  <-> ADRS: [STUDYID, USUBJID]
  <-> ADRSSWIM: [STUDYID, USUBJID]
  <-> ADLB: [STUDYID, USUBJID]
ADTR: [STUDYID, USUBJID, PARAMCD, AVISIT]
  --> ADSL: [STUDYID, USUBJID]
  <-> ADAE: [STUDYID, USUBJID]
  <-> ADEX: [STUDYID, USUBJID]
  <-> ADCM: [STUDYID, USUBJID]
  <-> ADTRWF: [STUDYID, USUBJID]
  <-> ADRS: [STUDYID, USUBJID]
  <-> ADRSSWIM: [STUDYID, USUBJID]
  <-> ADLB: [STUDYID, USUBJID]
ADTRWF: [STUDYID, USUBJID, PARAMCD, AVISIT]
  --> ADSL: [STUDYID, USUBJID]
  <-> ADAE: [STUDYID, USUBJID]
  <-> ADEX: [STUDYID, USUBJID]
  <-> ADCM: [STUDYID, USUBJID]
  <-> ADTR: [STUDYID, USUBJID]
  <-> ADRS: [STUDYID, USUBJID]
  <-> ADRSSWIM: [STUDYID, USUBJID]
  <-> ADLB: [STUDYID, USUBJID]
ADRS: [STUDYID, USUBJID, PARAMCD, AVISIT]
  --> ADSL: [STUDYID, USUBJID]
  <-> ADAE: [STUDYID, USUBJID]
  <-> ADEX: [STUDYID, USUBJID]
  <-> ADCM: [STUDYID, USUBJID]
  <-> ADTR: [STUDYID, USUBJID]
  <-> ADTRWF: [STUDYID, USUBJID]
  <-> ADRSSWIM: [STUDYID, USUBJID]
  <-> ADLB: [STUDYID, USUBJID]
ADRSSWIM: [STUDYID, USUBJID, PARAMCD, AVISIT]
  --> ADSL: [STUDYID, USUBJID]
  <-> ADAE: [STUDYID, USUBJID]
  <-> ADEX: [STUDYID, USUBJID]
  <-> ADCM: [STUDYID, USUBJID]
  <-> ADTR: [STUDYID, USUBJID]
  <-> ADTRWF: [STUDYID, USUBJID]
  <-> ADRS: [STUDYID, USUBJID]
  <-> ADLB: [STUDYID, USUBJID]
ADLB: [STUDYID, USUBJID, PARAMCD, AVISIT]
  --> ADSL: [STUDYID, USUBJID]
  <-> ADAE: [STUDYID, USUBJID]
  <-> ADEX: [STUDYID, USUBJID]
  <-> ADCM: [STUDYID, USUBJID]
  <-> ADTR: [STUDYID, USUBJID]
  <-> ADTRWF: [STUDYID, USUBJID]
  <-> ADRS: [STUDYID, USUBJID]
  <-> ADRSSWIM: [STUDYID, USUBJID] 

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @kartikeyakirar
The second is the old class which will be removed next week. In old classes update_keys_given_parent are run in a different moment so the keys are hardcoded in a list.
Both lists will work as expected, where the second one won't be here anymore.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it!

early-dev/app.R Outdated Show resolved Hide resolved
@vedhav
Copy link
Contributor

vedhav commented Nov 20, 2023

Please merge this PR #96 and merge the main into dev before merging this PR targeting thedev branch.

Copy link
Contributor

@kartikeyakirar kartikeyakirar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested all the applications, and none of them have encountered any failures in any module.

@gogonzo gogonzo merged commit ae76ecb into dev Nov 20, 2023
@gogonzo gogonzo deleted the 78_simplify_joinkeys@main branch November 20, 2023 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
6 participants