Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

Closes #98: align SAS rounding and update the calculation for ADSL.DURDIS #99

Merged
merged 11 commits into from
Apr 28, 2023
Merged

Closes #98: align SAS rounding and update the calculation for ADSL.DURDIS #99

merged 11 commits into from
Apr 28, 2023

Conversation

kaz462
Copy link
Collaborator

@kaz462 kaz462 commented Apr 21, 2023 •
edited
Loading

Added round_sas function and updated round to round_sas

  • differences when using round:

image

  • round_sas

image

@kaz462 kaz462 linked an issue Apr 21, 2023 that may be closed by this pull request
Resolve discrepancies in ADSL: align SAS rounding and update the calculation for ADSL.DURDIS #98
Closed
@kaz462 kaz462 requested a review from robertdevine April 21, 2023 20:14
@kaz462
Copy link
Collaborator Author

kaz462 commented Apr 24, 2023 •
edited
Loading

The DURDIS differences are not from rounding, but from the way duration is calculated (details in pharmaverse/admiral#1875)

Discrepancies are resolved by updating derive_vars_duration(..., out_unit = "months") to "days", and dividing it by 365.25/12

Remaining diff (due to original CDISC data):
image

@kaz462 kaz462 changed the title Closes #98: align SAS rounding Closes #98: align SAS rounding and update the calculation for ADSL.DURDIS Apr 24, 2023
DeclanHodges
DeclanHodges suggested changes Apr 25, 2023
View reviewed changes
Copy link

@DeclanHodges DeclanHodges left a comment

Choose a reason for hiding this comment

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

Hi @kaz462,
Good spot with the DURDIS discrepancy! Thanks for updating the code. Just one comment from me (see below).

laxamanaj
laxamanaj approved these changes Apr 28, 2023
View reviewed changes
Copy link
Collaborator

@laxamanaj laxamanaj left a comment

Choose a reason for hiding this comment

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

Thanks @kaz462 and @DeclanHodges for updating and reviewing.

@laxamanaj laxamanaj merged commit 876d011 into main Apr 28, 2023
@laxamanaj laxamanaj deleted the 98_sas_rounding branch April 28, 2023 17:09
@kaz462 kaz462 mentioned this pull request Aug 4, 2023
Merged
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@laxamanaj laxamanaj laxamanaj approved these changes

@DeclanHodges DeclanHodges DeclanHodges approved these changes

@bms63 bms63 Awaiting requested review from bms63

@thomas-neitmann thomas-neitmann Awaiting requested review from thomas-neitmann

@SHAESEN2 SHAESEN2 Awaiting requested review from SHAESEN2

@robertdevine robertdevine Awaiting requested review from robertdevine

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Resolve discrepancies in ADSL: align SAS rounding and update the calculation for ADSL.DURDIS
3 participants
@kaz462 @laxamanaj @DeclanHodges