Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

add pdt02 #345

Merged
merged 15 commits into from
Jan 23, 2023
Merged

add pdt02 #345

merged 15 commits into from
Jan 23, 2023

Conversation

6iris6
Copy link
Contributor

@6iris6 6iris6 commented Dec 21, 2022

close insightsengineering/chevron-tasks#55

@6iris6 6iris6 added the sme label Dec 21, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Dec 22, 2022

Unit Tests Summary

    1 files    23 suites   1m 7s ⏱️
104 tests   72 ✔️ 32 💤 0
228 runs  147 ✔️ 81 💤 0

Results for commit 4f50873.

♻️ This comment has been updated with latest results.

R/pdt02.R Show resolved Hide resolved
R/pdt02.R Outdated Show resolved Hide resolved
R/pdt02.R Show resolved Hide resolved
Copy link
Contributor

@barnett11 barnett11 left a comment

Choose a reason for hiding this comment

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

Thanks @6iris6 , some suggestions below,

  • DVREAS is showing a rogue (n) in it's description?
  • Column headers should read Primary Reason and Description
    Thanks

…ption for all patients column;updated column header
@6iris6
Copy link
Contributor Author

6iris6 commented Jan 12, 2023

Thanks @6iris6 , some suggestions below,

  • DVREAS is showing a rogue (n) in it's description?
  • Column headers should read Primary Reason and Description
    Thanks

Thanks @barnett11
1.The (n) may be from the last column, it's related to your console window length;
2. headers are updated.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 12, 2023

🧪 $Test coverage: 89.53%$

Code Coverage Summary

Filename                     Stmts    Miss  Cover    Missing
-------------------------  -------  ------  -------  ---------------------------------
R/aet01_aesi.R                 188       8  95.74%   39, 41-45, 272, 285
R/aet01.R                      330       6  98.18%   38, 240, 253, 316, 322, 554
R/aet02.R                      222       2  99.10%   136, 489
R/aet03.R                       85       0  100.00%
R/aet04.R                      104       6  94.23%   81-86
R/assertions.R                  36       0  100.00%
R/checks.R                      14       0  100.00%
R/chevron_tlg-S4class.R         21       0  100.00%
R/chevron_tlg-S4methods.R       42      16  61.90%   47-55, 90-98, 218-290
R/cmt01a.R                     191       0  100.00%
R/cmt02_pt.R                    52       0  100.00%
R/dmt01.R                       40       0  100.00%
R/dst01.R                      287       0  100.00%
R/dtht01.R                     104       0  100.00%
R/egt01.R                       46       0  100.00%
R/egt02.R                       60       0  100.00%
R/egt03.R                      129      85  34.11%   32-52, 111, 125-232, 291, 305-339
R/ext01.R                       81       8  90.12%   238-241, 245-248
R/gen_args.R                     1       1  0.00%    27
R/lbt01.R                       95       0  100.00%
R/lbt04.R                       52       1  98.08%   127
R/lbt05.R                       70       2  97.14%   121, 149
R/mht01.R                       72       2  97.22%   33-34
R/mng01.R                       93      12  87.10%   113, 117-120, 129-137, 182
R/pdt01.R                       61      38  37.70%   35-52, 108-150
R/pdt02.R                       68       0  100.00%
R/utils.R                      163     109  33.13%   70, 83-212
R/vst01.R                       48       0  100.00%
R/vst02.R                      102       3  97.06%   42, 120, 257
TOTAL                         2857     299  89.53%

Results for commit: e89f23ea9d7bb319e42d8faac330f622976e6675

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

@barnett11
Copy link
Contributor

Thanks @6iris6 , some suggestions below,

  • DVREAS is showing a rogue (n) in it's description?
  • Column headers should read Primary Reason and Description
    Thanks

Thanks @barnett11 1.The (n) may be from the last column, it's related to your console window length; 2. headers are updated.

I've looked and I'm pretty sure it's not wrapping, appears in every variation of text that i create for DVREAS?
image

@6iris6
Copy link
Contributor Author

6iris6 commented Jan 12, 2023

Thanks @6iris6 , some suggestions below,

  • DVREAS is showing a rogue (n) in it's description?
  • Column headers should read Primary Reason and Description
    Thanks

Thanks @barnett11 1.The (n) may be from the last column, it's related to your console window length; 2. headers are updated.

I've looked and I'm pretty sure it's not wrapping, appears in every variation of text that i create for DVREAS? image

Apologize, my misunderstanding. Do you want to remove (n) @barnett11 ? I got it from the TLG-C PDT02
https://docs.roche.com/#/tlg-catalog/devel/tables/safety/pdt02.html

@barnett11
Copy link
Contributor

Thanks @6iris6 , some suggestions below,

  • DVREAS is showing a rogue (n) in it's description?
  • Column headers should read Primary Reason and Description
    Thanks

Thanks @barnett11 1.The (n) may be from the last column, it's related to your console window length; 2. headers are updated.

I've looked and I'm pretty sure it's not wrapping, appears in every variation of text that i create for DVREAS? image

Apologize, my misunderstanding. Do you want to remove (n) @barnett11 ? I got it from the TLG-C PDT02 https://docs.roche.com/#/tlg-catalog/devel/tables/safety/pdt02.html

The (n) is not in the template so I'd advise to remove thanks @6iris6

@edelarua
Copy link
Contributor

edelarua commented Jan 16, 2023

Hi @barnett11, Iris is out of office so I will be finishing up this PR for her. I have removed the (n) from the table labels so this should be ready for another review now. Thanks!

@barnett11
Copy link
Contributor

Hi @edelarua
Thanks for this, unfortunately the layout still isn't quite right, and numbers not matching expected - the display should be as below, with DVREAS being shown as nn(xx.x%). I've added mockup to original request, and adding below also for reference,
image

@edelarua
Copy link
Contributor

Hi @edelarua Thanks for this, unfortunately the layout still isn't quite right, and numbers not matching expected - the display should be as below, with DVREAS being shown as nn(xx.x%).

Hi @barnett11, I have updated the value formats to match the mockup. Regarding the numbers not matching expected - could you send me sample expected output for syn_data (or other sample data) so I can compare values against it?

@barnett11
Copy link
Contributor

Hi @edelarua ,
I beleive DVREAS is counting all occurrences, and not patients - please see my test example below showing this,
image

@edelarua
Copy link
Contributor

edelarua commented Jan 17, 2023

@barnett11 I have opened a PR in tern (insightsengineering/tern#799) to fix the label/value formatting in summarize_num_patients. Once that is merged this should be properly formatted (labels & value formats). I will let you know once that is done.

Also, should the DVTERM counts be counting occurrences or patients?

EDIT: @barnett11 This is now done and the template should be outputting correctly. Let me know if any other changes are needed!

@edelarua edelarua dismissed barnett11’s stale review January 18, 2023 22:55

Changes have been implemented

@edelarua
Copy link
Contributor

@barnett11 this is ready for another review

@edelarua edelarua merged commit e0d1b41 into main Jan 23, 2023
@edelarua edelarua deleted the 245_pdt02@main branch January 23, 2023 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants