add nearest-value rounding support

[Melkiades]

Alternative work for #336. This PR does not need any change downstream.

Please, @gmbecker, let me know what you think!

@shajoezhu, @edelarua, and @ayogasekaram fyi

[github-actions]

Unit Tests Summary

  1 files    9 suites   16s ⏱️
 57 tests  57 ✅ 0 💤 0 ❌
384 runs  384 ✅ 0 💤 0 ❌

Results for commit 0b22140.

♻️ This comment has been updated with latest results.

[github-actions]

Code Coverage Summary

Filename             Stmts    Miss  Cover    Missing
-----------------  -------  ------  -------  ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
R/format_value.R       215      12  94.42%   105, 122-129, 208, 242, 448, 462, 470
R/generics.R           125      12  90.40%   142, 280-284, 486, 498, 529, 559, 667, 680, 701-708
R/labels.R              55       7  87.27%   51, 57, 66, 107, 133, 142, 146
R/matrix_form.R        692      41  94.08%   132, 393, 513-514, 606, 616-619, 637, 668, 758-759, 773-778, 808-811, 871-872, 961-962, 989, 1017, 1069, 1231, 1267, 1270-1274, 1324, 1372, 1375, 1379
R/mpf_exporters.R      277      20  92.78%   2, 100-102, 147, 183, 227, 230, 235, 415-421, 425, 428, 432, 480, 558
R/page_size.R           42       1  97.62%   219
R/pagination.R         758      52  93.14%   327-330, 435-450, 540, 593, 598, 639, 677-688, 764, 876-877, 899-908, 1046, 1097-1098, 1249, 1286-1290, 1306, 1313, 1395, 1530-1531, 1547-1548, 1562-1563
R/tostring.R           783      66  91.57%   88, 247, 302, 372, 405, 413, 449, 506-509, 545, 609-612, 618-622, 625-628, 635-640, 723-724, 864-865, 930-937, 987-991, 1060, 1113, 1132-1136, 1147, 1165, 1182, 1197, 1295, 1336, 1381, 1467, 1506, 1560, 1567
R/utils.R                3       0  100.00%
R/zzz.R                 28       6  78.57%   28-33
TOTAL                 2978     217  92.71%

Diff against main

Filename            Stmts    Miss  Cover
----------------  -------  ------  -------
R/format_value.R      +18       0  +0.51%
R/pagination.R         -1       0  -0.01%
R/zzz.R               +11       0  +13.87%
TOTAL                 +28       0  +0.07%

Results for commit: 0b22140

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

[github-actions]

Unit Test Performance Difference

Test Suite	$Status$	Time on main	$±Time$	$±Tests$	$±Skipped$	$±Failures$	$±Errors$
zzz	👶		$+0.05$	$+18$	$0$	$0$	$0$

Additional test case details

Test Suite	$Status$	Time on main	$±Time$	Test Case
zzz	👶		$+0.01$	Changing_rounding_type_works
zzz	👶		$+0.02$	Default_horizontal_separator_works
zzz	👶		$+0.02$	Default_rounding_type_setting_and_retrieval_work

Results for commit 55c7b8c

♻️ This comment has been updated with latest results.

[gmbecker]

@Melkiades I am extremely wary of having a global option that controls rounding behavior, because it damages reproducibility.

Imagine I get an edge-case number (one that rounds to a different value under sas and iec rounding) and I send my data and script to someone else, but the option was set to sas rounding somewhere else, e.g. in another output's script. When they run the script on the data they will get an output that performs iec rounding when rendered, and so won't agree with what I got.

I'd much prefer that formatters not provide an option like that. chevron could (although it doesn't need to because the default behavior remains the iec rounding that roche uses) and our J&J exporters will all be setting round_type to sas, but I still think it should be an argument that is passed down and its up to the user or calling code to set it correctly.

[shajoezhu]

Hi @gmbecker , I also would prefer @Melkiades 's implementation, as I suggested in the email last year. I think using an option is the most efficient way for now.

@Melkiades 's change only appeared in three files, and very limited function, whereas the proposal in #337 has a lot of downstream changes in functions (5 files), and will have to change rtables as well (https://github.com/insightsengineering/rtables/pull/990/files), we have several packages (internally as well, use toString and matrix_form functions), the change you had proposed is going to have a lot of downstream impact on us for changing, as well as maintaining.

We have very established pipeline on this, and I am afraid the risk is very high for us to accept your PR as it is.

Would it be helpful for jnj to consider using a chevron like approach to control these options, as I understand it is still at an exploring stage for jnj, I feel you guys have more flexibilities, and open minded with these proposals. Thanks! @khatril @iaugusty fyi

[gmbecker]

@shajoezhu @Melkiades let me try again to explain why I think this approach is a mistake.

This software stack is used to generate outputs submitted to the FDA and other regulatory authorities. Code is also submitted, AFAIK, so that FDA personnel can check the outputs. If the outputs the fda checkers get different numbers when they run the code, that would be, to put it mildly, "not good". Having a global option (which, I should note, there's no meaningful way to require it be set in any particular script), opens the possibility of that happening.

@Melkiades 's change only appeared in three files, and very limited function, whereas the proposal in #337 has a lot of downstream changes in functions (5 files), and will have to change rtables as well (https://github.com/insightsengineering/rtables/pull/990/files), we have several packages (internally as well, use toString and matrix_form functions), the change you had proposed is going to have a lot of downstream impact on us for changing, as well as maintaining.

I prepared the relevant downstream PRs, and the change is fully backwards compatible, so you guys won't need to do anything if you don't want to support sas-style rounding in your downstream uses.

We have very established pipeline on this, and I am afraid the risk is very high for us to accept your PR as it is.

Maybe we should discuss this in a meeting, because I don't see the risk, the default value for round_type is the backwards compatible iec rounding so it shouldn't break any code (and doesn't cause failures in scda.test for me).