nlmixr2 / rxode2et

https://nlmixr2.github.io/rxode2et/
1 stars 1 forks source link

Clinical Trial utilites #27

Closed OmarAshkar closed 1 year ago

OmarAshkar commented 1 year ago

Hello, I'd like to contribute some functions that I usually use for building CTS. The draft here is quick and dirty and still need tweaks and tests, but I am just asking if I can proceed and get some feedback.

to_trial_duration is a function that modify the event table to start from baseline to certain trial duration. The idea is that in disease progression the baseline usually does not start at zero and will vary based on ID.

Please consider it and let me know..

Best Regards, Omar

mattfidler commented 1 year ago

Thank you for the idea; it sounds interesting.

A couple of things to have it integrated into rxode2et and therefore rxode2:

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 91.66% and project coverage change: +0.54% :tada:

Comparison is base (4f5cf69) 61.50% compared to head (8608ba1) 62.04%.

:exclamation: Current head 8608ba1 differs from pull request most recent head 7aca156. Consider uploading reports for the commit 7aca156 to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #27 +/- ## ========================================== + Coverage 61.50% 62.04% +0.54% ========================================== Files 20 21 +1 Lines 3959 3971 +12 ========================================== + Hits 2435 2464 +29 + Misses 1524 1507 -17 ``` | [Files Changed](https://app.codecov.io/gh/nlmixr2/rxode2et/pull/27?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=nlmixr2) | Coverage Δ | | |---|---|---| | [R/CTS.R](https://app.codecov.io/gh/nlmixr2/rxode2et/pull/27?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=nlmixr2#diff-Ui9DVFMuUg==) | `91.66% <91.66%> (ø)` | | ... and [2 files with indirect coverage changes](https://app.codecov.io/gh/nlmixr2/rxode2et/pull/27/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=nlmixr2)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

mattfidler commented 1 year ago

Also this should have a successful check after the tests are integrated, including the code style check with CodeFactor which is currently failing.

OmarAshkar commented 1 year ago

Hi Matt,

I have fixed the comments, but I need to ask for few things:-

  1. creating event table with et() always requires id to be integer, but character or factor will not work, is it a desirable behavior?
  2. After installing and loading rxode2, I cannot find the exported function. But loading rxode2et directly works. Isn't rxode2 load it automatically?
  3. Things like checkmake and data.table has been used a lot here. But I cannot find them imported in the namespace definition.

Thank you, Omar

mattfidler commented 1 year ago

creating event table with et() always requires id to be integer, but character or factor will not work, is it a desirable behavior?

This is a default behavior. As changing it would possibly create a large amount of work with little benefit, I am going to keep it as is for now.

After installing and loading rxode2, I cannot find the exported function. But loading rxode2et directly works. Isn't rxode2 load it automatically?

No, it has to be re-exported from rxode2et otherwise it isn't seen.

Things like checkmake and data.table has been used a lot here. But I cannot find them imported in the namespace definition.

They imported elsewhere so they don't really change the import structure.

mattfidler commented 1 year ago

I think you need to put data.table into the DESCRIPTION if you are going to import it. This will help the checks succeed

OmarAshkar commented 1 year ago

It was quite complicated, but I have removed all dependencies now.

mattfidler commented 1 year ago

Merged, thanks @oimsaA