search

pharmaverse / sdtm.oak

An EDC and Data Standard agnostic SDTM data transformation engine that automates the transformation of raw clinical data in ODM format to SDTM based on standard mapping algorithms
https://pharmaverse.github.io/sdtm.oak/
Apache License 2.0
22 stars 6 forks source link

# 16 calculate_study_day #20

Closed yli110-stat697 closed 4 months ago

yli110-stat697 commented 8 months ago

close #16

github-actions[bot] commented 8 months ago

Code Coverage

Package Line Rate Health
sdtm.oak 86% ✔
Summary 86% (349 / 408) ✔
yli110-stat697 commented 8 months ago

Hi @ShiyuC can you take a look at this design and see if it's okay? If so, I can start to add documentation and unit tests

ShiyuC commented 7 months ago

Hi @ShiyuC can you take a look at this design and see if it's okay? If so, I can start to add documentation and unit tests

@yli110-stat697 Hi Rosemary, thanks for creating this PR. It looks good to me. Feel free to move on with documentation and unit test at your convenience

ShiyuC commented 7 months ago

Hi @yli110-stat697! Could you create a test within the testthat folder so that we could run the function with your test? Thanks!

yli110-stat697 commented 7 months ago

Hi @yli110-stat697! Could you create a test within the testthat folder so that we could run the function with your test? Thanks!

Hi yes I can, do we agree on the design already?

rammprasad commented 5 months ago

@yli110-stat697 - I think we have good review comments. Once you complete the below tasks and have a green pipeline, we can do a quick re-review and merge to the main

  1. merge main to this FB
  2. Resolve conflicts and fix pipeline failures.
yli110-stat697 commented 5 months ago
  1. Resolve conflicts and fix pipeline failures.

Hi @rammprasad there were some consistent lintr issues 1) the message said do not use nested ifelse(), see https://github.com/pharmaverse/sdtm.oak/actions/runs/7865236485/job/21457935296?pr=20

but I think the logic was copied from internal oak package, maybe we need an update there too? BTW I tried to use mapply() but lintr suggested that the function was not desired either...

2) can we have the lintr package installed in renv so that we can replicate the error in our local environment instead of waiting for the pipeline to check?

@ramiromagno FYI

yli110-stat697 commented 4 months ago

EDIT, EDIT: Now that I've check how study day is calculated, I think it is actually simpler to implement. Please take a look at my commit.

Thanks @ramiromagno I think it looks good except that the condition should have equal sign. BTW, you can use "Suggest" functionality instead of pushing commit to show your suggestion. Developer can directly commit your "suggestion". Because forgetting to pull other people's commit usually causes merge conflict locally.

Screenshot 2024-02-19 at 9 55 35 AM

ramiromagno commented 4 months ago

EDIT, EDIT: Now that I've check how study day is calculated, I think it is actually simpler to implement. Please take a look at my commit.

Thanks @ramiromagno I think it looks good except that the condition should have equal sign. BTW, you can use "Suggest" functionality instead of pushing commit to show your suggestion. Developer can directly commit your "suggestion". Because forgetting to pull other people's commit usually causes merge conflict locally.

Screenshot 2024-02-19 at 9 55 35 AM

Hi @yli110-stat697 : Thanks for the tip. Will do next time (sorry for the inconvenience!).

yli110-stat697 commented 4 months ago

Hi @galachad my pipeline failed due to pandoc installation error. I belive this has nothing to do with my code. How do I fix this?