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

intended use #105

Merged
merged 45 commits into from
Dec 9, 2021
Merged

intended use #105

merged 45 commits into from
Dec 9, 2021

Conversation

waddella
Copy link
Contributor

@waddella waddella commented Dec 7, 2021

including data pre-processing

@waddella waddella requested a review from BFalquet December 7, 2021 17:26
@insightsengineering insightsengineering deleted a comment from github-actions bot Dec 7, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Dec 7, 2021

File Coverage Missing
All files 0%
R/aet02.R 0% 57-427
R/aet03.R 0% 45-151
R/aet04.R 0% 55-163
R/assertthat.R 0% 3
R/dmt01.R 0% 48-110
R/dst01.R 0% 6-540
R/ext01.R 0% 42-183
R/gen_args.R 0% 20
R/lbt01.R 0% 45-115
R/standard_data_preprocessing.R 0% 29-325
R/utils.R 0% 15-384

Minimum allowed coverage is 80%

Generated by 🐒 cobertura-action against 6018f0d

@wangh107
Copy link
Contributor

wangh107 commented Dec 8, 2021

@waddella
Is it possible to add example of how extra_args should be defined in the variable lopo in the intended_use vignettes?
If I need to change the arm and overall label, how would you suggest we specify them?
Screen Shot 2021-12-07 at 6 48 21 PM

Copy link
Contributor

@BFalquet BFalquet left a comment

Choose a reason for hiding this comment

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

Minor comments otherwise ready to merge. Thank you for the hard work.

Comment on lines 49 to 50
Hence these layouts define a table pre-data. Note that pruning and sorting are currently not performed in the layout space in `rtables` but rather on the actual table objects.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would add that in general, we try to make the layout function independent from the data, in the sense that they shouldn't take a dm or part of a dm object as an argument. Indeed, there is frequently a temptation to extract data from the dm object (for instances labels) inside the layout function and this is a bad idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see my suggestion above

* `chevron` exports the layout functions for each table creating *tlg-function*
* these layouts are useful to understand how table is created and what variables are required in the data to create a table using the layout
* the table creation functions focus the main part of their code on building, sorting and pruning the table

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a word about headers and footnotes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

please see my my suggestion below

Comment on lines +32 to +40
1. they produce a narrow defined output (currently standards in Roche GDS). Note, that the naming convention
`<gds template id>_<i>` indicates that a Roche GDS defined standard may have different implementations. Or,
alternatively, a GDS template id can be regarded as a *guideline* and the function name in `chevron` as a
*standard*.
1. have very few arguments to modify the standard. Generally, arguments may change the structure of the table (arm
variable, which variables are summarized) but not parameterize the cell content (i.e. alpha-level for p-value).
1. have always the first argument `adam_db` which is the collection of ADaM datasets (`ADSL`, `ADAE`,
`ADRS`, etc.). Please read the *The adam_db Argument* vignette in this package for more details.
1. have a `.study` argument, read the *The .study argument* vignette for more detail.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
1. they produce a narrow defined output (currently standards in Roche GDS). Note, that the naming convention
`<gds template id>_<i>` indicates that a Roche GDS defined standard may have different implementations. Or,
alternatively, a GDS template id can be regarded as a *guideline* and the function name in `chevron` as a
*standard*.
1. have very few arguments to modify the standard. Generally, arguments may change the structure of the table (arm
variable, which variables are summarized) but not parameterize the cell content (i.e. alpha-level for p-value).
1. have always the first argument `adam_db` which is the collection of ADaM datasets (`ADSL`, `ADAE`,
`ADRS`, etc.). Please read the *The adam_db Argument* vignette in this package for more details.
1. have a `.study` argument, read the *The .study argument* vignette for more detail.
1. they produce a narrow defined output (currently standards in Roche GDS). Note, that the naming convention
`<gds template id>_<i>` indicates that a Roche GDS defined standard may have different implementations. Or,
alternatively, a GDS template id can be regarded as a *guideline* and the function name in `chevron` as a
*standard*.
2. have very few arguments to modify the standard. Generally, arguments may change the structure of the table (arm
variable, which variables are summarized) but not parameterize the cell content (i.e. alpha-level for p-value).
3. have always the first argument `adam_db` which is the collection of ADaM datasets (`ADSL`, `ADAE`,
`ADRS`, etc.). Please read the *The adam_db Argument* vignette in this package for more details.
4. have a `.study` argument, read the *The .study argument* vignette for more detail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the numbering happens automatically in markdown

validate_dm(adam_study_data)
```

Note that the keys are defined in the data analysis plan at Roche.
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe a link with an example if it is possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will leave it out as it is an internal resource. This is something to revisit though.

Copy link
Contributor Author

@waddella waddella left a comment

Choose a reason for hiding this comment

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

@BFalquet thanks for reviewing, please have a look at my suggested changes

validate_dm(adam_study_data)
```

Note that the keys are defined in the data analysis plan at Roche.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will leave it out as it is an internal resource. This is something to revisit though.

Comment on lines +32 to +40
1. they produce a narrow defined output (currently standards in Roche GDS). Note, that the naming convention
`<gds template id>_<i>` indicates that a Roche GDS defined standard may have different implementations. Or,
alternatively, a GDS template id can be regarded as a *guideline* and the function name in `chevron` as a
*standard*.
1. have very few arguments to modify the standard. Generally, arguments may change the structure of the table (arm
variable, which variables are summarized) but not parameterize the cell content (i.e. alpha-level for p-value).
1. have always the first argument `adam_db` which is the collection of ADaM datasets (`ADSL`, `ADAE`,
`ADRS`, etc.). Please read the *The adam_db Argument* vignette in this package for more details.
1. have a `.study` argument, read the *The .study argument* vignette for more detail.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the numbering happens automatically in markdown

Comment on lines 49 to 50
Hence these layouts define a table pre-data. Note that pruning and sorting are currently not performed in the layout space in `rtables` but rather on the actual table objects.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see my suggestion above

* `chevron` exports the layout functions for each table creating *tlg-function*
* these layouts are useful to understand how table is created and what variables are required in the data to create a table using the layout
* the table creation functions focus the main part of their code on building, sorting and pruning the table

Copy link
Contributor Author

Choose a reason for hiding this comment

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

please see my my suggestion below

@waddella waddella merged commit 7b4ef11 into main Dec 9, 2021
@waddella waddella deleted the 96_vignettes branch December 9, 2021 08:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants