Remove dynamic domain determination #193

elimillera · 2023-11-27T23:19:13Z

Changes Description

First draft of removing they dynamic determination of .df names. This is replaced with the domain argument and a new function for setting the df_arg attribute called xportr_domain_name. Closes #182

Task List

codecov · 2023-11-27T23:22:11Z

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (6a74578) 100.00% compared to head (466d7ef) 99.82%.
Report is 6 commits behind head on main.

❗ Current head 466d7ef differs from pull request most recent head e3a31b1. Consider uploading reports for the commit e3a31b1 to get more accurate results

Files	Patch %	Lines
R/metadata.R	66.66%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##              main     #193      +/-   ##
===========================================
- Coverage   100.00%   99.82%   -0.18%     
===========================================
  Files           12       12              
  Lines          595      582      -13     
===========================================
- Hits           595      581      -14     
- Misses           0        1       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

vedhav

Good job reflecting the domain changes across all the xportr functions. Left a few small comments. Will have a closer look at the vignette and docs after this.

R/metadata.R

tests/testthat/test-metadata.R

tests/testthat/test-type.R

R/utils-xportr.R

R/type.R

bms63 · 2023-12-05T16:46:21Z

@elimillera what is your ETA on this PR? We need to merge into #190 to finish this one off

…r into 182-remove-df-expr

bms63

I think this update warrants a note in the news/changelog!!

averissimo

~~There's a styler CI error~~
~~Minor comment to update comments in R/*~~
Should this change be added to NEWS.md? (it seems like a big breaking change for those that use this functionality)

R/df_label.R

averissimo

The tests/testthat/test-pipe.R should be renamed as it no longer test pipes independently.

Some options:

rename to tests/testthat/test-domain.R
same rename, but move xporter_domain_name() to its own R/domain.R
- following {testthat} convention to test active file
move the tests to tests/testthat/test-metadata.R

R/length.R

averissimo

(sorry for the multiple review, but I've been noticing as I take a look at the assertion PR)

As it is, it allows for domain to be NULL with all the xportr functions, which I think itsn't the desired behavior.

Previously it wasn't the case as it was fallbacking to the dynamic determination.

WDYT @elimillera ?

elimillera · 2023-12-18T21:52:26Z

(sorry for the multiple review, but I've been noticing as I take a look at the assertion PR)

As it is, it allows for domain to be NULL with all the xportr functions, which I think itsn't the desired behavior.

Previously it wasn't the case as it was fallbacking to the dynamic determination.

WDYT @elimillera ?

I actually think this is ok. If the user has really basic metadata that just has one domain. I think xportr is safe to assume that no filtering needs to be done for domain. I do see the arguments for requiring users explicitly supply it though

R/metadata.R

README.Rmd

bms63 · 2023-12-19T13:41:17Z

I think some updates need to be made to the vignettes so the R-CMD checks pass

Co-authored-by: Ben Straub <[email protected]>

EeethB · 2023-12-21T16:32:58Z

Could you add the new function to the pkgdown site too?

averissimo · 2023-12-21T16:58:59Z

I was now testing all examples with empty domain and the "empty domain" problem appears on all other functions (with exception of df_label)

😕

Do you want to deal this on this PR or deal with this on a new one?

Expand to see failing (reproducible) code

library(xportr)
#> ℹ Loading xportr

# format ----------------------------------------------------------------------

adsl <- data.frame(
  USUBJID = c(1001, 1002, 1003),
  BRTHDT = c(1, 1, 2)
)

metadata <- data.frame(
  dataset = c("adsl", "adsl"),
  variable = c("USUBJID", "BRTHDT"),
  format = c(NA, "DATE9.")
)

adsl <- xportr_format(adsl, metadata)
#> Error in `dplyr::filter()`:
#> ℹ In argument: `dataset == domain & !is.na(format)`.
#> Caused by error:
#> ! `..1` must be of size 2 or 1, not size 0.
#> Backtrace:
#>      ▆
#>   1. ├─xportr::xportr_format(adsl, metadata)
#>   2. │ └─metadata %>% ... at xportr/R/format.R:76:5
#>   3. ├─dplyr::filter(., !!sym(domain_name) == domain & !is.na(!!sym(format_name)))
#>   4. ├─dplyr:::filter.data.frame(., !!sym(domain_name) == domain & !is.na(!!sym(format_name))) at dplyr/R/filter.R:119:3
#>   5. │ └─dplyr:::filter_rows(.data, dots, by) at dplyr/R/filter.R:134:3
#>   6. │   └─dplyr:::filter_eval(...) at dplyr/R/filter.R:149:3
#>   7. │     ├─base::withCallingHandlers(...) at dplyr/R/filter.R:217:3
#>   8. │     └─mask$eval_all_filter(dots, env_filter) at dplyr/R/filter.R:217:3
#>   9. │       └─dplyr (local) eval() at dplyr/R/data-mask.R:99:7
#>  10. ├─dplyr:::dplyr_internal_error(...)
#>  11. │ └─rlang::abort(class = c(class, "dplyr:::internal_error"), dplyr_error_data = data) at dplyr/R/conditions.R:194:3
#>  12. │   └─rlang:::signal_abort(cnd, .file) at rlang/R/cnd-abort.R:379:3
#>  13. │     └─base::signalCondition(cnd) at rlang/R/cnd-abort.R:850:5
#>  14. └─dplyr (local) `<fn>`(`<dpl:::__>`)
#>  15.   └─rlang::abort(message, class = error_class, parent = parent, call = error_call) at dplyr/R/conditions.R:235:5

# df_label --------------------------------------------------------------------

# adsl <- data.frame(
#   USUBJID = c(1001, 1002, 1003),
#   SITEID = c(001, 002, 003),
#   AGE = c(63, 35, 27),
#   SEX = c("M", "F", "M")
# )
#
# metadata <- data.frame(
#   dataset = c("adsl", "adae"),
#   label = c("Subject-Level Analysis", "Adverse Events Analysis")
# )
#
# adsl <- xportr_df_label(adsl, metadata)

# label -----------------------------------------------------------------------

adsl <- data.frame(
  USUBJID = c(1001, 1002, 1003),
  SITEID = c(001, 002, 003),
  AGE = c(63, 35, 27),
  SEX = c("M", "F", "M")
)

metadata <- data.frame(
  dataset = "adsl",
  variable = c("USUBJID", "SITEID", "AGE", "SEX"),
  label = c("Unique Subject Identifier", "Study Site Identifier", "Age", "Sex")
)

adsl <- xportr_label(adsl, metadata)
#> Error in `dplyr::filter()`:
#> ℹ In argument: `dataset == domain`.
#> Caused by error:
#> ! `..1` must be of size 4 or 1, not size 0.
#> Backtrace:
#>      ▆
#>   1. ├─xportr::xportr_label(adsl, metadata)
#>   2. │ └─metadata %>% dplyr::filter(!!sym(domain_name) == domain) at xportr/R/label.R:92:5
#>   3. ├─dplyr::filter(., !!sym(domain_name) == domain)
#>   4. ├─dplyr:::filter.data.frame(., !!sym(domain_name) == domain) at dplyr/R/filter.R:119:3
#>   5. │ └─dplyr:::filter_rows(.data, dots, by) at dplyr/R/filter.R:134:3
#>   6. │   └─dplyr:::filter_eval(...) at dplyr/R/filter.R:149:3
#>   7. │     ├─base::withCallingHandlers(...) at dplyr/R/filter.R:217:3
#>   8. │     └─mask$eval_all_filter(dots, env_filter) at dplyr/R/filter.R:217:3
#>   9. │       └─dplyr (local) eval() at dplyr/R/data-mask.R:99:7
#>  10. ├─dplyr:::dplyr_internal_error(...)
#>  11. │ └─rlang::abort(class = c(class, "dplyr:::internal_error"), dplyr_error_data = data) at dplyr/R/conditions.R:194:3
#>  12. │   └─rlang:::signal_abort(cnd, .file) at rlang/R/cnd-abort.R:379:3
#>  13. │     └─base::signalCondition(cnd) at rlang/R/cnd-abort.R:850:5
#>  14. └─dplyr (local) `<fn>`(`<dpl:::__>`)
#>  15.   └─rlang::abort(message, class = error_class, parent = parent, call = error_call) at dplyr/R/conditions.R:235:5

# length ----------------------------------------------------------------------

adsl <- data.frame(
  USUBJID = c(1001, 1002, 1003),
  BRTHDT = c(1, 1, 2)
)

metadata <- data.frame(
  dataset = c("adsl", "adsl"),
  variable = c("USUBJID", "BRTHDT"),
  length = c(10, 8)
)

adsl <- xportr_length(adsl, metadata)
#> Error in `filter()`:
#> ℹ In argument: `dataset == domain`.
#> Caused by error:
#> ! `..1` must be of size 2 or 1, not size 0.
#> Backtrace:
#>      ▆
#>   1. ├─xportr::xportr_length(adsl, metadata)
#>   2. │ └─metadata %>% filter(!!sym(domain_name) == domain) at xportr/R/length.R:99:5
#>   3. ├─dplyr::filter(., !!sym(domain_name) == domain)
#>   4. ├─dplyr:::filter.data.frame(., !!sym(domain_name) == domain) at dplyr/R/filter.R:119:3
#>   5. │ └─dplyr:::filter_rows(.data, dots, by) at dplyr/R/filter.R:134:3
#>   6. │   └─dplyr:::filter_eval(...) at dplyr/R/filter.R:149:3
#>   7. │     ├─base::withCallingHandlers(...) at dplyr/R/filter.R:217:3
#>   8. │     └─mask$eval_all_filter(dots, env_filter) at dplyr/R/filter.R:217:3
#>   9. │       └─dplyr (local) eval() at dplyr/R/data-mask.R:99:7
#>  10. ├─dplyr:::dplyr_internal_error(...)
#>  11. │ └─rlang::abort(class = c(class, "dplyr:::internal_error"), dplyr_error_data = data) at dplyr/R/conditions.R:194:3
#>  12. │   └─rlang:::signal_abort(cnd, .file) at rlang/R/cnd-abort.R:379:3
#>  13. │     └─base::signalCondition(cnd) at rlang/R/cnd-abort.R:850:5
#>  14. └─dplyr (local) `<fn>`(`<dpl:::__>`)
#>  15.   └─rlang::abort(message, class = error_class, parent = parent, call = error_call) at dplyr/R/conditions.R:235:5

# order -----------------------------------------------------------------------

adsl <- data.frame(
  BRTHDT = c(1, 1, 2),
  STUDYID = c("mid987650", "mid987650", "mid987650"),
  TRT01A = c("Active", "Active", "Placebo"),
  USUBJID = c(1001, 1002, 1003)
)

metadata <- data.frame(
  dataset = c("adsl", "adsl", "adsl", "adsl"),
  variable = c("STUDYID", "USUBJID", "TRT01A", "BRTHDT"),
  order = 1:4
)

adsl <- xportr_order(adsl, metadata)
#> Error in `dplyr::filter()`:
#> ℹ In argument: `dataset == domain & !is.na(order)`.
#> Caused by error:
#> ! `..1` must be of size 4 or 1, not size 0.
#> Backtrace:
#>      ▆
#>   1. ├─xportr::xportr_order(adsl, metadata)
#>   2. │ └─metadata %>% ... at xportr/R/order.R:95:5
#>   3. ├─dplyr::filter(., !!sym(domain_name) == domain & !is.na(!!sym(order_name)))
#>   4. ├─dplyr:::filter.data.frame(., !!sym(domain_name) == domain & !is.na(!!sym(order_name))) at dplyr/R/filter.R:119:3
#>   5. │ └─dplyr:::filter_rows(.data, dots, by) at dplyr/R/filter.R:134:3
#>   6. │   └─dplyr:::filter_eval(...) at dplyr/R/filter.R:149:3
#>   7. │     ├─base::withCallingHandlers(...) at dplyr/R/filter.R:217:3
#>   8. │     └─mask$eval_all_filter(dots, env_filter) at dplyr/R/filter.R:217:3
#>   9. │       └─dplyr (local) eval() at dplyr/R/data-mask.R:99:7
#>  10. ├─dplyr:::dplyr_internal_error(...)
#>  11. │ └─rlang::abort(class = c(class, "dplyr:::internal_error"), dplyr_error_data = data) at dplyr/R/conditions.R:194:3
#>  12. │   └─rlang:::signal_abort(cnd, .file) at rlang/R/cnd-abort.R:379:3
#>  13. │     └─base::signalCondition(cnd) at rlang/R/cnd-abort.R:850:5
#>  14. └─dplyr (local) `<fn>`(`<dpl:::__>`)
#>  15.   └─rlang::abort(message, class = error_class, parent = parent, call = error_call) at dplyr/R/conditions.R:235:5

# type ------------------------------------------------------------------------

metadata <- data.frame(
  dataset = "test",
  variable = c("Subj", "Param", "Val", "NotUsed"),
  type = c("numeric", "character", "numeric", "character"),
  format = NA
)

.df <- data.frame(
  Subj = as.character(123, 456, 789),
  Different = c("a", "b", "c"),
  Val = c("1", "2", "3"),
  Param = c("param1", "param2", "param3")
)

df2 <- xportr_type(.df, metadata)
#> Error in `filter()`:
#> ℹ In argument: `dataset == domain`.
#> Caused by error:
#> ! `..1` must be of size 4 or 1, not size 0.
#> Backtrace:
#>      ▆
#>   1. ├─xportr::xportr_type(.df, metadata)
#>   2. │ └─metadata %>% filter(!!sym(domain_name) == domain) at xportr/R/type.R:117:5
#>   3. ├─dplyr::filter(., !!sym(domain_name) == domain)
#>   4. ├─dplyr:::filter.data.frame(., !!sym(domain_name) == domain) at dplyr/R/filter.R:119:3
#>   5. │ └─dplyr:::filter_rows(.data, dots, by) at dplyr/R/filter.R:134:3
#>   6. │   └─dplyr:::filter_eval(...) at dplyr/R/filter.R:149:3
#>   7. │     ├─base::withCallingHandlers(...) at dplyr/R/filter.R:217:3
#>   8. │     └─mask$eval_all_filter(dots, env_filter) at dplyr/R/filter.R:217:3
#>   9. │       └─dplyr (local) eval() at dplyr/R/data-mask.R:99:7
#>  10. ├─dplyr:::dplyr_internal_error(...)
#>  11. │ └─rlang::abort(class = c(class, "dplyr:::internal_error"), dplyr_error_data = data) at dplyr/R/conditions.R:194:3
#>  12. │   └─rlang:::signal_abort(cnd, .file) at rlang/R/cnd-abort.R:379:3
#>  13. │     └─base::signalCondition(cnd) at rlang/R/cnd-abort.R:850:5
#>  14. └─dplyr (local) `<fn>`(`<dpl:::__>`)
#>  15.   └─rlang::abort(message, class = error_class, parent = parent, call = error_call) at dplyr/R/conditions.R:235:5

^{Created on 2023-12-21 with reprex v2.0.2}

bms63 · 2023-12-21T18:58:33Z

I was now testing all examples with empty domain and the "empty domain" problem appears on all other functions (with exception of df_label)

😕

Do you want to deal this on this PR or deal with this on a new one?

Expand to see failing (reproducible) code

I think we should handle this issue in this PR.

elimillera · 2023-12-29T21:08:21Z

I was now testing all examples with empty domain and the "empty domain" problem appears on all other functions (with exception of df_label)

😕

Do you want to deal this on this PR or deal with this on a new one?

Expand to see failing (reproducible) code

Thanks for that @averissimo I made some updates and added some tests for that. It will just ignore the domain if it is null here. Is that more what you had in mind to fix it?

EeethB · 2024-01-09T14:28:00Z

@elimillera I saw the xportr_domain_name() function is like xportr_metadata() but only for the domain. Could we just make metadata an optional argument in xportr_metadata() and not need an additional function?

R/metadata.R

README.Rmd

Remove dynamic domain determination

251ac9a

averissimo mentioned this pull request Nov 30, 2023

Closes #175 Adds assertions to exported functions #190

Merged

18 tasks

vedhav reviewed Dec 5, 2023

View reviewed changes

R/metadata.R Outdated Show resolved Hide resolved

tests/testthat/test-metadata.R Show resolved Hide resolved

tests/testthat/test-type.R Show resolved Hide resolved

R/utils-xportr.R Show resolved Hide resolved

R/type.R Show resolved Hide resolved

Update docs and tests per review

5f6e0dc

elimillera marked this pull request as ready for review December 6, 2023 13:53

elimillera added 3 commits December 6, 2023 07:53

Merge branch 'main' into 182-remove-df-expr

2891d93

update style

c5edb1d

Merge branch '182-remove-df-expr' of github.com:atorus-research/xport…

d0c7fc6

…r into 182-remove-df-expr

bms63 reviewed Dec 7, 2023

View reviewed changes

bms63 linked an issue Dec 7, 2023 that may be closed by this pull request

Remove dynamic determination of .df argument name for filtering metadata #182

Closed

EeethB linked an issue Dec 7, 2023 that may be closed by this pull request

Bug: xportr_df_label() doesn't seem to work unless domain argument set #180

Closed

averissimo reviewed Dec 7, 2023

View reviewed changes

R/df_label.R Show resolved Hide resolved

averissimo added 2 commits December 8, 2023 10:58

fix: updates comments

c73780b

style: rm extra empty line

17c7f14

averissimo requested changes Dec 8, 2023

View reviewed changes

averissimo reviewed Dec 15, 2023

View reviewed changes

R/length.R Show resolved Hide resolved

averissimo reviewed Dec 15, 2023

View reviewed changes

elimillera added 3 commits December 18, 2023 21:36

Add news entry

49b44e8

Remove unneeded tests

9da5288

doc update for domain argument

e7d96c9

Merge branch 'main' into 182-remove-df-expr

b02aa0e

elimillera requested review from averissimo, bms63 and vedhav December 18, 2023 21:54

bms63 reviewed Dec 19, 2023

View reviewed changes

R/metadata.R Outdated Show resolved Hide resolved

bms63 reviewed Dec 19, 2023

View reviewed changes

README.Rmd Outdated Show resolved Hide resolved

elimillera and others added 2 commits December 19, 2023 10:28

Update R/metadata.R

a5bd527

Co-authored-by: Ben Straub <[email protected]>

Update with PR comments

dc832f3

elimillera requested a review from bms63 December 19, 2023 16:30

elimillera added 2 commits December 19, 2023 16:46

passing R CMD Check

e378cbd

fix coverage

8c5e0ce

elimillera added 3 commits December 29, 2023 20:27

Updates with PR comment

6138a14

Merge branch 'main' into 182-remove-df-expr

1769235

Revert bad merge

8db698e

Remove function and update xportr_metadata to allow for domain setting

466d7ef

elimillera requested a review from EeethB January 10, 2024 17:16

EeethB reviewed Jan 10, 2024

View reviewed changes

R/metadata.R Show resolved Hide resolved

bms63 reviewed Jan 11, 2024

View reviewed changes

README.Rmd Outdated Show resolved Hide resolved

averissimo and others added 2 commits January 16, 2024 16:40

tests: add missing coverage

e3a31b1

Update readme per comments

601209f

elimillera merged commit 48f8130 into main Jan 16, 2024
11 checks passed

elimillera deleted the 182-remove-df-expr branch January 16, 2024 15:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove dynamic domain determination #193

Remove dynamic domain determination #193

elimillera commented Nov 27, 2023 •

edited

Loading

codecov bot commented Nov 27, 2023 •

edited

Loading

vedhav left a comment

bms63 commented Dec 5, 2023

bms63 left a comment

averissimo left a comment •

edited

Loading

averissimo left a comment

averissimo left a comment

elimillera commented Dec 18, 2023

bms63 commented Dec 19, 2023

EeethB commented Dec 21, 2023

averissimo commented Dec 21, 2023 •

edited

Loading

bms63 commented Dec 21, 2023

elimillera commented Dec 29, 2023

EeethB commented Jan 9, 2024 •

edited

Loading

Remove dynamic domain determination #193

Remove dynamic domain determination #193

Conversation

elimillera commented Nov 27, 2023 • edited Loading

Changes Description

Task List

codecov bot commented Nov 27, 2023 • edited Loading

Codecov Report

vedhav left a comment

Choose a reason for hiding this comment

bms63 commented Dec 5, 2023

bms63 left a comment

Choose a reason for hiding this comment

averissimo left a comment • edited Loading

Choose a reason for hiding this comment

averissimo left a comment

Choose a reason for hiding this comment

averissimo left a comment

Choose a reason for hiding this comment

elimillera commented Dec 18, 2023

bms63 commented Dec 19, 2023

EeethB commented Dec 21, 2023

averissimo commented Dec 21, 2023 • edited Loading

bms63 commented Dec 21, 2023

elimillera commented Dec 29, 2023

EeethB commented Jan 9, 2024 • edited Loading

elimillera commented Nov 27, 2023 •

edited

Loading

codecov bot commented Nov 27, 2023 •

edited

Loading

averissimo left a comment •

edited

Loading

averissimo commented Dec 21, 2023 •

edited

Loading

EeethB commented Jan 9, 2024 •

edited

Loading