search

RConsortium / rtrs-wg

R Tables for Regulatory Submissions Working Group
https://rconsortium.github.io/rtrs-wg/
62 stars 17 forks source link

Add in AE, ConMed, and Dispo tables for {tfrmt} #100

Closed statasaurus closed 1 year ago

dmurdoch commented 1 year ago

The CI builds failed because of some missing packages in rtrs-wg/tools/fakepkg/DESCRIPTION. I've just added some others (including random.cdisc.data, the one that is reported as failing), but it doesn't currently have GSK-Biostatistics/tfrmt listed in the remotes, so it will try to get the CRAN version. You should merge in the recent changes from the main branch, and if you want the Github tfrmt version, add it to Remotes in the DESCRIPTION file mentioned above.

dmurdoch commented 1 year ago

I've built the book on my system with your changes. Here are some comments:

  1. Readers might not know what an AdAM is (I don't). Maybe use the same link as was used in Chap 1?
  2. Proportions (e.g. 0.9) are shown instead of percentages (e.g. 91%) (I think in all tfrmt tables).
  3. The header marker is wrong in the concomitant table, so it shows up as 4.6 when it should be 4.5.6.
  4. In the Disposition section (4.7 in yours, but should be 4.6), there's a lot of inconsistency in the formatting of the tables; I think we need to talk about that in the next Zoom meeting. But I think there are some problems specific to tfrmt e.g. some missing death causes (suicide, unknown).
statasaurus commented 1 year ago

I've built the book on my system with your changes. Here are some comments:

  1. Readers might not know what an AdAM is (I don't). Maybe use the same link as was used in Chap 1?
  2. Proportions (e.g. 0.9) are shown instead of percentages (e.g. 91%) (I think in all tfrmt tables).
  3. The header marker is wrong in the concomitant table, so it shows up as 4.6 when it should be 4.5.6.
  4. In the Disposition section (4.7 in yours, but should be 4.6), there's a lot of inconsistency in the formatting of the tables; I think we need to talk about that in the next Zoom meeting. But I think there are some problems specific to tfrmt e.g. some missing death causes (suicide, unknown).

1) ADaM's are the data format required by the FDA for submission, so I think it is okay to assume most people are somewhat familiar, but I did take you suggestion and added the link just incase. 2) The issue was an old version of tfrmt, I have change the description to get the latest from GitHub 3) I have corrected 4) The issue was I used the wrong column, I have since corrected

dmurdoch commented 1 year ago

Thanks for the quick changes. I have merged the changes into the main branch. A couple of small changes I think you should deal with sometime:

  1. In the introduction, you mark the package name as {tfrmt} and the braces show up in the text. Other packages use backticks "`", and that just formats the name as code.
  2. In 4.4.8, the group sizes (e.g. (N=134) in the rtables example) aren't displayed.

And one other change that I think is an overall formatting change, not up to you: the base pipe symbols "|>" are being displayed as triangles, whereas the magrittr pipe symbols are shown as they are entered, i.e. "%>%". I find the triangles to be kind of weird looking, and would prefer to display them as text too.