Closes #142 issue_142_updated to account for DT, DTM, TM variables #145

adchan11 · 2023-05-31T03:41:13Z

Thank you for your Pull Request!

We have developed a Pull Request template to aid you and our reviewers. Completing the below tasks helps to ensure our reviewers can maximize their time on your code as well as making sure the xportr codebase remains robust and consistent.

The scope of `{xportr}`

{xportr}'s scope is to enable R users to write out submission compliant xpt files that can be delivered to a Health Authority or to downstream validation software programs. We see labels, lengths, types, ordering and formats from a dataset specification object (SDTM and ADaM) as being our primary focus. We also see messaging and warnings to users around applying information from the specification file as a primary focus. Please make sure your Pull Request meets this scope of {xportr}. If your Pull Request moves beyond this scope, please get in touch with the {xportr} team on slack or create an issue to discuss.

Please check off each task box as an acknowledgment that you completed the task. This checklist is part of the Github Action workflows and the Pull Request will not be merged into the devel branch until you have checked off each task.

Changes Description

#142

do not convert DT, DT, and DTM variables with a format specified in the metacore specs (e.g. date9., datetime20.) to numeric, which will cause a 10 year difference when reading it back by read_xpt

Task List

R/type.R

cpiraux · 2023-06-01T08:34:19Z

R/type.R

        attributes(.df[[i]]) <<- orig_attributes
+        } else {
+          attributes(.df[[i]]) <- NULL
+        }


The data are still converted to numeric R class. The attribute should numeric but the R class should stay date/dttm/time. The numeric date variables should not go through ".df[[i]] <<- as.numeric(.df[[i]])"

Thanks for your updates. Numeric dates are still coerced to dbl, it should not be the case.

@cpiraux can you give an example dataframe where this happens and what the expected result should be?

It seems that it is actually not coerced but the message is still present. I used df and metadata in test-type for the review:

This should be fixed now @cpiraux

@elimillera can you take a look too? I've implemented your suggestion in today's meeting. Thanks.

I didn't use a metacore object but a dataframe for the metadata

metadata <- data.frame( dataset = c("adsl", "adsl", "adsl", "adsl"), variable = c("USUBJID", "DMDTC", "RFICDT", "RFICDTM"), type = c("text", "date", "integer", "integer"), format = c(NA, NA, "date9.", "datetime15.") )

Jump in the conversation

Went through and everything looked ok from the code side. I would rely on others to make sure it behaves as expected. I'll be sure to update this in the type.R documentation

I updated the testthat for this function and I see a new issue where in line 205 of test-type.R, the original df and xpt file have different timezones.

── Failure (test-type.R:208:3): xportr_type: date variables are not converted to numeric ──
df$RFICDTM (actual) not equal to df_xpt$RFICDTM (expected).

actual: 1490824800
expected: 1490832000

I checked the documentation and it comes from this parameter in the write_xpt function of haven:

Is this something to be concerned about? @cpiraux @elimillera @kaz462

Thanks for looking into this! Timezone should be ignored. As the default value in haven for adjust_tz = TRUE, I think it is okay. I open dfdates.xpt in SAS and dates are okay. Is it possible to ignore the timezone when you compare the dates in testthat?

@cpiraux test updated to expect that the as_character() of each datetime are equal so that timezone is ignored.

cpiraux · 2023-06-01T08:36:43Z

Could you add a test for numeric date variables in test-type.R?

adchan11 · 2023-06-01T17:11:45Z

@cpiraux testthat updated

cpiraux

Thanks for your updates. Numeric dates are still coerced to dbl, it should not be the case.

cpiraux · 2023-06-07T08:16:31Z

I did a test using a df iso metacore object as metadata. The numeric dates have been coerced to character without giving a message:

metadata <- data.frame(
  dataset = c("adsl", "adsl", "adsl", "adsl"),
  variable = c("USUBJID", "DMDTC", "RFICDT", "RFICDTM"),
  type = c("text", "date", "integer", "integer"),
  format = c(NA, NA, "date9.", "datetime15.")
)

adsl_original <- tibble::tribble(
  ~USUBJID, ~DMDTC, ~RFICDT, ~RFICDTM,
  "test1", "2017-03-30", "2017-03-30", "2017-03-30",
  "test2", "2017-01-08", "2017-01-08", "2017-01-08"
)

adsl_original$RFICDT <- as.Date(adsl_original$RFICDT)

adsl_original$RFICDTM <- as.POSIXct(adsl_original$RFICDTM)

adsl_xpt2 <- adsl_original %>%
  xportr_type(metadata) # Coerce variable type to match spec - stop if any mismatch

metacore does not contains any row:

and then type.x is _character

cpiraux · 2023-06-07T09:30:40Z

Another issue that @kaz462 pointed out is when a date in the data frame is numeric, but the type in the metadata belongs to the character type. In this case, the date is not coerced to a character, and no message is provided to inform the user that the types in the metadata and data are different.

df <- data.frame(RFDTC = as.Date('2017-03-30'), RFICDT = as.Date('2017-03-30'), RFICDTM = as.POSIXct('2017-03-30'))


metacore_meta <- suppressWarnings(
  metacore::metacore(
    var_spec = data.frame(
      variable = c("RFDTC", "RFICDT", "RFICDTM"),
      type = c("date", "integer", "integer"),
      label = c("RFDTC Label", "RFICDT Label", "RFICDTM Label"),
      length = c(20, 8, 8),
      common = NA_character_,
      format = c(NA, "date9.", "datetime20.")
    )
  )
)

processed_df <- xportr_type(df, metacore_meta)

processed_df

In type.R, meta_ordered:

adchan11 · 2023-06-08T19:16:41Z

@cpiraux Pushed new changes. Please let me know your feedback.

@elimillera could you take a look when you get a chance? I'm finding the function more complicated more than I thought and I feel like I'm just putting 'workarounds' to ensure that Celine's issues are fixed while still passing the test-type.R. E.g. the meta_ordered logic is getting longer and longer to account for the discrepancies that Celine is finding

elimillera

Went through and everything looked ok from the code side. I would rely on others to make sure it behaves as expected. I'll be sure to update this in the type.R documentation

…xportr into issue_142_dates

cpiraux

Great, thanks for the updates. It seems to work pretty well. I only see coerced message when no coercion is done. Could you check this?

adsl_original <- tibble::tribble(
  ~USUBJID, ~DMDTC, ~RFICDT, ~RFICDTM,
  "test1", "2017-03-30", "2017-03-30", "2017-03-30",
  "test2", "2017-01-08", "2017-01-08", "2017-01-08"
)

adsl_original$DMDTC <- as.Date(adsl_original$DMDTC)
adsl_original$RFICDT <- as.Date(adsl_original$RFICDT)

adsl_original$RFICDTM <- as.POSIXct(adsl_original$RFICDTM)

adsl_xpt <- adsl_original %>%
  xportr_type(metacore)

adchan11 · 2023-06-14T09:34:32Z

@cpiraux When I run this code, I get one coercion that seems to make sense:

adsl_original <- tibble::tribble(
  ~USUBJID, ~DMDTC, ~RFICDT, ~RFICDTM,
  "test1", "2017-03-30", "2017-03-30", "2017-03-30",
  "test2", "2017-01-08", "2017-01-08", "2017-01-08"
)

metadata <- data.frame(
  dataset = c("adsl", "adsl", "adsl", "adsl"),
  variable = c("USUBJID", "DMDTC", "RFICDT", "RFICDTM"),
  type = c("text", "date", "integer", "integer"),
  format = c(NA, NA, "date9.", "datetime15.")
)

adsl_original$DMDTC <- as.Date(adsl_original$DMDTC)
adsl_original$RFICDT <- as.Date(adsl_original$RFICDT)

adsl_original$RFICDTM <- as.POSIXct(adsl_original$RFICDTM)

adsl_xpt <- adsl_original %>%
  xportr_type(metadata)

── Variable type mismatches found. ──

✔ 1 variables coerced
> View(metadata)
> View(adsl_original)
> lapply(adsl_original, class)
$USUBJID
[1] "character"

$DMDTC
[1] "Date"

$RFICDT
[1] "Date"

$RFICDTM
[1] "POSIXct" "POSIXt" 

> lapply(adsl_xpt, class)
$USUBJID
[1] "character"

$DMDTC
[1] "character"

$RFICDT
[1] "Date"

$RFICDTM
[1] "POSIXct" "POSIXt"

Can you provide the code for your metacore object that gives you this issue where coercion shouldn't happen and the expected outcome?

cpiraux · 2023-06-14T11:48:47Z

@cpiraux When I run this code, I get one coercion that seems to make sense:

Can you provide the code for your metacore object that gives you this issue where coercion shouldn't happen and the expected outcome?

Sorry I did not give the code with an issue. Please use this one:

metadata <- data.frame(
  dataset = c("adsl", "adsl", "adsl", "adsl"),
  variable = c("USUBJID", "DMDTC", "RFICDT", "RFICDTM"),
  type = c("text", "date", "integer", "integer"),
  format = c(NA, NA, "date9.", "datetime15.")
)

adsl_original <- tibble::tribble(
  ~USUBJID, ~DMDTC, ~RFICDT, ~RFICDTM,
  "test1", "2017-03-30", "2017-03-30", "2017-03-30",
  "test2", "2017-01-08", "2017-01-08", "2017-01-08"
)


adsl_original$RFICDT <- as.Date(adsl_original$RFICDT)
adsl_original$RFICDTM <- as.POSIXct(adsl_original$RFICDTM)

adsl_xpt2 <- adsl_original %>%
  xportr_type(metadata)

adchan11 · 2023-06-14T14:46:55Z

@cpiraux When I run this code, I get one coercion that seems to make sense:

Can you provide the code for your metacore object that gives you this issue where coercion shouldn't happen and the expected outcome?

Sorry I did not give the code with an issue. Please use this one:
metadata <- data.frame(
  dataset = c("adsl", "adsl", "adsl", "adsl"),
  variable = c("USUBJID", "DMDTC", "RFICDT", "RFICDTM"),
  type = c("text", "date", "integer", "integer"),
  format = c(NA, NA, "date9.", "datetime15.")
)

adsl_original <- tibble::tribble(
  ~USUBJID, ~DMDTC, ~RFICDT, ~RFICDTM,
  "test1", "2017-03-30", "2017-03-30", "2017-03-30",
  "test2", "2017-01-08", "2017-01-08", "2017-01-08"
)


adsl_original$RFICDT <- as.Date(adsl_original$RFICDT)
adsl_original$RFICDTM <- as.POSIXct(adsl_original$RFICDTM)

adsl_xpt2 <- adsl_original %>%
  xportr_type(metadata)

@cpiraux I fixed this issue but now we get the other issue where if a DTC variable is a date class, it is not coerced to character. But is it necessary for DTC variables to be coerced to character? When I use xportr_write and read in the xpt file, the date value is still the same so I feel like it is ok to leave it as it is.

Please let me know your thoughts. If you have any suggestions for improved logic in the code for xportr_type, that would be great as there is a lot of different scenarios with dates/datetimes that is making this quite confusing.

df <- data.frame(RFDTC = as.Date('2017-03-30'), RFICDT = as.Date('2017-03-30'), RFICDTM = as.POSIXct('2017-03-30'))


metacore_meta <- suppressWarnings(
  metacore::metacore(
    var_spec = data.frame(
      variable = c("RFDTC", "RFICDT", "RFICDTM"),
      type = c("date", "integer", "integer"),
      label = c("RFDTC Label", "RFICDT Label", "RFICDTM Label"),
      length = c(20, 8, 8),
      common = NA_character_,
      format = c(NA, "date9.", "datetime20.")
    )
  )
)

processed_df <- xportr_type(df, metacore_meta)

xportr_write(processed_df, file.path(system.file("extdata", package="xportr"), "dfdates2.xpt"))
df_xpt <- read_xpt(file.path(system.file("extdata", package="xportr"), "dfdates2.xpt"))

cpiraux · 2023-06-14T15:45:43Z

@cpiraux I fixed this issue but now we get the other issue where if a DTC variable is a date class, it is not coerced to character. But is it necessary for DTC variables to be coerced to character? When I use xportr_write and read in the xpt file, the date value is still the same so I feel like it is ok to leave it as it is.

Please let me know your thoughts. If you have any suggestions for improved logic in the code for xportr_type, that would be great as there is a lot of different scenarios with dates/datetimes that is making this quite confusing.

It would be great to have both. --DTC variables coerced to chr when they have an other type and no message when there is no coercion.

If it is not possible to have it for this release, I suggest to have no message when no coercion and add an issue for --DTC variables to be done for the next release. What do you think?

cpiraux

We can keep it like this and resolve the coercion from --DTC variable to for the next release. Could you add an issue for this?

adchan11 · 2023-06-15T12:40:35Z

It would be great to have both. --DTC variables coerced to chr when they have an other type and no message when there is no coercion.

Done! Thanks for your review.

bms63 · 2023-06-15T17:07:17Z

LGTM!

updated to account for DT, DTM, TM variables

c0f84b0

adchan11 requested review from kaz462 and cpiraux May 31, 2023 03:41

cpiraux requested changes Jun 1, 2023

View reviewed changes

updates

12fd07b

Chan, Adrian {MDBT~Mississauga} added 2 commits June 1, 2023 19:14

fixed

2f0e783

fixed

713f3d3

cpiraux requested changes Jun 5, 2023

View reviewed changes

adchan11 requested a review from elimillera June 6, 2023 15:56

updates

0fa5e68

bms63 and others added 2 commits June 8, 2023 11:46

Merge branch 'devel' into issue_142_dates

e45cb28

updates

3583b24

cpiraux mentioned this pull request Jun 12, 2023

Closes #84 xportr deep dive vignette #141

Merged

14 tasks

elimillera added 2 commits June 13, 2023 14:50

Passing R CMD Check, linting, styling

a772092

Merge branch 'devel' into issue_142_dates

008386c

elimillera approved these changes Jun 13, 2023

View reviewed changes

elimillera and others added 3 commits June 13, 2023 11:44

Merge branch 'devel' into issue_142_dates

027fcff

updated test

b9ea9e7

Merge branch 'issue_142_dates' of https://github.com/atorus-research/…

8808bc1

…xportr into issue_142_dates

cpiraux reviewed Jun 14, 2023

View reviewed changes

updates

839bffe

updates

00dcfe7

cpiraux approved these changes Jun 15, 2023

View reviewed changes

adchan11 closed this Jun 15, 2023

adchan11 reopened this Jun 15, 2023

adchan11 mentioned this pull request Jun 15, 2023

Bug: --DTC variable coercion to character #161

Closed

elimillera added 3 commits June 15, 2023 09:04

Merge branch 'devel' into issue_142_dates

7d2c4e2

clean up files, remove tibble dependancy on tests

0efc1b0

Lint and style

412c75d

bms63 linked an issue Jun 15, 2023 that may be closed by this pull request

Dates are not correctly derived (SAS vs R) #142

Closed

chore: #142 news update

ddc813b

bms63 changed the title ~~issue_142_updated to account for DT, DTM, TM variables~~ Closes #142 issue_142_updated to account for DT, DTM, TM variables Jun 15, 2023

bms63 merged commit 79550f9 into devel Jun 15, 2023

bms63 deleted the issue_142_dates branch June 15, 2023 17:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Closes #142 issue_142_updated to account for DT, DTM, TM variables #145

Closes #142 issue_142_updated to account for DT, DTM, TM variables #145

adchan11 commented May 31, 2023

cpiraux Jun 1, 2023

adchan11 Jun 1, 2023

adchan11 Jun 5, 2023

cpiraux Jun 6, 2023

adchan11 Jun 6, 2023

cpiraux Jun 7, 2023

bms63 Jun 8, 2023

adchan11 Jun 14, 2023

cpiraux Jun 14, 2023

adchan11 Jun 14, 2023

cpiraux commented Jun 1, 2023 •

edited

Loading

adchan11 commented Jun 1, 2023

cpiraux left a comment

cpiraux commented Jun 7, 2023

cpiraux commented Jun 7, 2023

adchan11 commented Jun 8, 2023

elimillera left a comment

cpiraux left a comment

adchan11 commented Jun 14, 2023

cpiraux commented Jun 14, 2023

adchan11 commented Jun 14, 2023 •

edited

Loading

cpiraux commented Jun 14, 2023

cpiraux left a comment

adchan11 commented Jun 15, 2023

bms63 commented Jun 15, 2023

Closes #142 issue_142_updated to account for DT, DTM, TM variables #145

Closes #142 issue_142_updated to account for DT, DTM, TM variables #145

Conversation

adchan11 commented May 31, 2023

Thank you for your Pull Request!

The scope of {xportr}

Changes Description

Task List

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpiraux commented Jun 1, 2023 • edited Loading

adchan11 commented Jun 1, 2023

cpiraux left a comment

Choose a reason for hiding this comment

cpiraux commented Jun 7, 2023

cpiraux commented Jun 7, 2023

adchan11 commented Jun 8, 2023

elimillera left a comment

Choose a reason for hiding this comment

cpiraux left a comment

Choose a reason for hiding this comment

adchan11 commented Jun 14, 2023

cpiraux commented Jun 14, 2023

adchan11 commented Jun 14, 2023 • edited Loading

cpiraux commented Jun 14, 2023

cpiraux left a comment

Choose a reason for hiding this comment

adchan11 commented Jun 15, 2023

bms63 commented Jun 15, 2023

The scope of `{xportr}`

cpiraux commented Jun 1, 2023 •

edited

Loading

adchan11 commented Jun 14, 2023 •

edited

Loading