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

Cautiously integrate/introduce expanded and more elastic standards for summaries' formats #994

Closed
Melkiades opened this issue Jun 29, 2023 · 6 comments · Fixed by #1051
Closed
Assignees

Comments

@Melkiades
Copy link
Contributor

Melkiades commented Jun 29, 2023

Function to be added: https://github.com/insightsengineering/chevron/blob/833caa90c6469f5f5a4c74fbd2fdbf2ef13e238c/R/rtables_utils.R#L399

Originally proposed @clarkliming

For the moment, I would add it as a possibility, then we can roll out older standards gradually.

@clarkliming
Copy link
Contributor

@barnett11 could you please evaluate the formats provided are good or not?

@clarkliming
Copy link
Contributor

by the way, may I ask if it is too much to store the format in every cell? or, after the row is created, use the format in whole row?

@Melkiades
Copy link
Contributor Author

by the way, may I ask if it is too much to store the format in every cell? or, after the row is created, use the format in whole row?

I do not know what exactly you mean but both things are possible. It is the main difference between using only in_rows and rcell. Of course they can be combined. Always lowest level format has precedence

@clarkliming
Copy link
Contributor

oh, what I mean is that if we only want to update the formats for whole row, shall we still, use make_afun to do so? (actually never mind this; adding formats to cell or row both works. I was just thinking that the storage (too many repetitions of storing formats if every cell has a format function)

@Melkiades
Copy link
Contributor Author

To add this I thought about using different lists of defaults and transforming it in a list. @BFalquet, we can talk about this if you can

@Melkiades
Copy link
Contributor Author

The PR related to this, @clarkliming, does not add the possibility of using other standards for now. It is a large refactoring in how the defaults are stored and so on. For the moment, adding specific formats is very easy with .formats so I think it is fine to keep tlg and chevron defaults in separate places

Melkiades added a commit that referenced this issue Sep 11, 2023
…els -> addition of auto for DMT01 (#1051)

Fixes #994 (for now)

Still todo:

- [x] tests for filtering
- [x] Examples
- [x] Evaluate the effect of having a list instead of a character vector
(it was the second, I am more inclined towards first)
- [x] start deprecation of `summary_formats`, `summary_labels`
- [x] discuss: consider changes in `summary_custom` (to discuss with
@edelarua)
- [x] wontdo: replace it within `count_occurences` and
`summarize_num_patients` (once reworked, wont do for now)
- [x] wontdo: consider using automatic function name getter for calling
function (related to previous. Not necessary until reworked)
- [x] wontdo: start deprecating other defaults (not related for now)

---------

Signed-off-by: Davide Garolini <dgarolini@gmail.com>
Signed-off-by: Davide Garolini <davide.garolini@roche.com>
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Emily de la Rua <59304861+edelarua@users.noreply.github.com>
Co-authored-by: 27856297+dependabot-preview[bot]@users.noreply.github.com <27856297+dependabot-preview[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants