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.

Sign up for GitHub

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

Jump to bottom

merge fix #83

Merged
merged 5 commits into from
Jul 13, 2022
Merged

merge fix #83

merged 5 commits into from
Jul 13, 2022

Conversation

gogonzo
Copy link
Contributor

@gogonzo gogonzo commented Jul 12, 2022

Two fixes:

  • fix duplicated ADSL_FILTERED <- ADSL in the get_rcode
    image

  • fix assertion of merge_expression_module to allow single data extract to be a list of extracts.

@gogonzo gogonzo added the core label Jul 12, 2022
gogonzo
gogonzo commented Jul 12, 2022
View reviewed changes
R/merge_datasets.R
Comment on lines -83 to -91
filtered_data_call <- lapply(selector_datanames, function(i) {
logger::log_trace("merge_datasets { paste0(i, \"_FILTERED\") } assigned.")
call(
"<-",
as.name(paste0(i, "_FILTERED")),
as.name(i)
)
})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there should be no filter calls in the merge_datasets as they are created in FilteredData.

gogonzo
gogonzo commented Jul 12, 2022
View reviewed changes
R/merge_expression_module.R
@@ -146,7 +146,7 @@ merge_expression_module <- function(datasets,
id = "merge_id") {
logger::log_trace("merge_expression_module called with: { paste(names(datasets), collapse = ', ') } datasets.")

checkmate::assert_list(data_extract, types = "data_extract_spec", names = "named")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be compatible with:

teal.transform/R/data_merge_module.R

Line 103 in 801b8be

checkmate::assert_list(data_extract)

teal.transform/R/data_extract_module.R

Line 578 in 801b8be

checkmate::assert_list(data_extract, names = "named")

data_extract (multiple) can be a list of lists of data_extract_spec objects. Because single data_extract can be also a list

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we also need to check that if it is a list then that inner list is only data_extrac_specs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, I've added more assertions

@github-actions
Copy link
Contributor

github-actions bot commented Jul 12, 2022 •
edited
Loading

Unit Tests Summary

    1 files    24 suites   49s ⏱️
195 tests 194 ✔️ 1 💤 0 ❌
674 runs  673 ✔️ 1 💤 0 ❌

Results for commit 5a139fb.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented Jul 12, 2022 •
edited
Loading

Code Coverage Summary

Filename                          Stmts    Miss  Cover    Missing
------------------------------  -------  ------  -------  ---------------------------------------------------------------------
R/all_choices.R                       1       0  100.00%
R/call_utils.R                      156     124  20.51%   14-23, 64, 66, 68, 107-431
R/check_selector.R                   31       0  100.00%
R/choices_labeled.R                 202      61  69.80%   60, 71, 76, 87, 103, 221-225, 229-234, 264-277, 390-391, 393, 425-473
R/choices_selected.R                 81      11  86.42%   201-229, 260
R/column_functions.R                  3       3  0.00%    13-16
R/data_extract_datanames.R           32       8  75.00%   9-13, 64-66
R/data_extract_filter_module.R       92      11  88.04%   75-82, 84, 87-88, 141
R/data_extract_module.R             255      61  76.08%   3, 121, 126, 143, 146-151, 153, 172-175, 203-249, 445, 450, 481, 581
R/data_extract_read_module.R        122      13  89.34%   28, 32-35, 102-107, 116, 133
R/data_extract_select_module.R       32      18  43.75%   31-48
R/data_extract_single_module.R       53       2  96.23%   29, 42
R/data_extract_spec.R                32       0  100.00%
R/data_merge_module.R                59      15  74.58%   101-118
R/filter_spec.R                     186       1  99.46%   373
R/format_data_extract.R              16       1  93.75%   49
R/get_dplyr_call.R                  299       0  100.00%
R/get_merge_call.R                  285      30  89.47%   29-36, 47, 210-219, 372, 388-400
R/include_css_js.R                    5       0  100.00%
R/input_checks.R                     11       2  81.82%   18-19
R/merge_data_utils.R                  2       0  100.00%
R/merge_datasets.R                  135       6  95.56%   72, 220-224
R/merge_expression_module.R          47       1  97.87%   152
R/resolve_delayed.R                  16       0  100.00%
R/resolve.R                         114      44  61.40%   229-312
R/select_spec.R                      49       8  83.67%   149, 213-220
R/utils.R                            15      11  26.67%   26-39
R/zzz.R                               2       2  0.00%    2-3
TOTAL                              2333     433  81.44%

Results for commit: 8595e04

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

@nikolas-burkoff nikolas-burkoff self-assigned this Jul 12, 2022
nikolas-burkoff
nikolas-burkoff approved these changes Jul 12, 2022
View reviewed changes
Copy link
Contributor

@nikolas-burkoff nikolas-burkoff left a comment

Choose a reason for hiding this comment

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

A small comment but yup this looks good

R/merge_expression_module.R
@@ -146,7 +146,7 @@ merge_expression_module <- function(datasets,
id = "merge_id") {
logger::log_trace("merge_expression_module called with: { paste(names(datasets), collapse = ', ') } datasets.")

checkmate::assert_list(data_extract, types = "data_extract_spec", names = "named")
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we also need to check that if it is a list then that inner list is only data_extrac_specs?

@gogonzo gogonzo merged commit 3e7a563 into main Jul 13, 2022
@gogonzo gogonzo deleted the 26_new_chunks@main branch July 13, 2022 07:58
@nikolas-burkoff
Copy link
Contributor

@gogonzo I think this PR broke the merge and extract-merge vignettes in teal.transform

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikolas-burkoff nikolas-burkoff nikolas-burkoff approved these changes

Assignees

@nikolas-burkoff nikolas-burkoff

Labels
core
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gogonzo @nikolas-burkoff