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

phase out of scda and intro of {pharmaverseadam} #75

Merged
merged 14 commits into from
Dec 13, 2023

Conversation

Melkiades
Copy link
Contributor

Fixes #74

@Melkiades Melkiades added enhancement New feature or request sme labels Nov 1, 2023
Copy link
Contributor

github-actions bot commented Nov 1, 2023

Unit Tests Summary

    1 files  112 suites   3m 39s ⏱️
250 tests 239 ✔️   11 💤 0
504 runs  255 ✔️ 249 💤 0

Results for commit da0d01e.

♻️ This comment has been updated with latest results.

edelarua
edelarua previously approved these changes Nov 1, 2023
Copy link
Contributor

@edelarua edelarua left a comment

Choose a reason for hiding this comment

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

Lgtm! Just one suggestion since I think you accidentally deleted a word here.

DESCRIPTION Outdated Show resolved Hide resolved
Melkiades and others added 3 commits November 2, 2023 16:39
Co-authored-by: Emily de la Rua <59304861+edelarua@users.noreply.github.com>
Signed-off-by: Davide Garolini <dgarolini@gmail.com>
…gineering/scda.test into 74_pharmaverseadam_trial@main

# Conflicts:
#	DESCRIPTION
DESCRIPTION Outdated Show resolved Hide resolved
Signed-off-by: Davide Garolini <dgarolini@gmail.com>
Copy link
Contributor

@zdz2101 zdz2101 left a comment

Choose a reason for hiding this comment

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

Hello! Just trying to learn some of the codebase here as I onboard onto sme, mostly small comments/musings for now

R/scda.test.R Outdated Show resolved Hide resolved
NAMESPACE Show resolved Hide resolved
tests/testthat/test-table_dmt01.R Show resolved Hide resolved
Melkiades and others added 2 commits November 9, 2023 09:35
Co-authored-by: Zelos Zhu <zelos.zhu@atorusresearch.com>
Signed-off-by: Davide Garolini <dgarolini@gmail.com>
Co-authored-by: Zelos Zhu <zelos.zhu@atorusresearch.com>
Signed-off-by: Davide Garolini <dgarolini@gmail.com>
@edelarua edelarua requested review from edelarua and removed request for edelarua November 23, 2023 01:47
@edelarua edelarua dismissed their stale review November 23, 2023 01:47

PR in progress

@Melkiades
Copy link
Contributor Author

@zdz2101 @edelarua @ayogasekaram @khatril @shajoezhu This PR can be merged only with regards to taking out scda, scda.2022 and replacing them with direct calls to cache from random.cdisc.data.

The trials regarding admiraldata are to be suspended imo as there are many missing labels (I understand it is a virtue of generality). Still, those being in adsl bring a lot of added pre-processing that I do not think is particularly interesting (i.e. adding labels to each instance).

If this is a priority, we can merge it w/o admiraldata. I tested it and {rcd} works fine

Copy link
Contributor

github-actions bot commented Nov 29, 2023

Unit Test Performance Difference

Test Suite $Status$ Time on main $±Time$ $±Tests$ $±Skipped$ $±Failures$ $±Errors$
pagination_table 💔 $14.00$ $+1.95$ $0$ $0$ $0$ $0$
table_aet02 💔 $9.05$ $+1.42$ $0$ $0$ $0$ $0$
table_aet04 💔 $13.73$ $+1.90$ $0$ $0$ $0$ $0$
table_aet04_pi 💔 $8.33$ $+1.31$ $0$ $0$ $0$ $0$
table_dmt01 💔 $2.54$ $+3.29$ $0$ $0$ $0$ $0$
table_ent 💔 $17.31$ $+2.26$ $0$ $0$ $0$ $0$
table_lbt13 💔 $12.95$ $+1.72$ $0$ $0$ $0$ $0$
table_lbt14 💔 $8.88$ $+1.94$ $0$ $0$ $0$ $0$
table_vst01 💔 $0.31$ $+1.08$ $+1$ $0$ $0$ $0$
Additional test case details
Test Suite $Status$ Time on main $±Time$ Test Case
table_dmt01 💔 $0.70$ $+3.14$ DMT01_variant_1_is_produced_correctly
table_vst01 💔 $0.31$ $+1.07$ VST01_default_variant_is_produced_correctly

Results for commit b36cf5a

♻️ This comment has been updated with latest results.

Signed-off-by: Joe Zhu <sha.joe.zhu@gmail.com>
@shajoezhu shajoezhu enabled auto-merge (squash) December 13, 2023 08:17
Copy link
Contributor

@shajoezhu shajoezhu left a comment

Choose a reason for hiding this comment

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

lgtm! thanks guys, I think the changes look good. We should start moving on this. this PR is a good start to remove dependencies on scda

@shajoezhu shajoezhu merged commit c070af4 into main Dec 13, 2023
26 checks passed
@shajoezhu shajoezhu deleted the 74_pharmaverseadam_trial@main branch December 13, 2023 08:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request sme
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[pharmaverseadam]: Update templates (deaths/dem/disp)
6 participants