fix PPSTRESN

[Shaakon35]

Issue

Closes #561

Description

I fixed three columns so that the numeric always match the character variable

(@Shaakon35 if possible explain here more what is the rationale behind the solution; why this works and/or what was the part of the code producing the problem)

Definition of Done

For PP/ADPP, it is expected based on CDISC rules that the result variables in numeric (PPSTRESN) and character (PPSTRESC) matches in length and share exactly the same values for the PP/ADPP objects. Therefore: PPSTRESN = PPSTRESC.

When no custom units are specified, is also expected that ``PPORRES = PPSTRESN = PPSTRESC`.

How to test

Run NCA with dummy data, download ADPP and PP to check all these variables are exactly the same value/length: PPORRES = PPSTRESN = PPSTRESC

Contributor checklist

• Code passes lintr checks
• Code passes all unit tests
• New logic covered by unit tests
• New logic is documented
• App or package changes are reflected in NEWS
• Package version is incremented
• R script works with the new implementation (if applicable)
• Settings upload works with the new implementation (if applicable)

[Gero1999]

Hey @Shaakon35 this one is a bit difficult to assess because the issue is no longer in main neither. The original issue if you remember was with the xpt files (the other formats were fine). And might be no longer happening because the dummy data has been changed and is now more polished

[Shaakon35]

Hey @Shaakon35 this one is a bit difficult to assess because the issue is no longer in main neither. The original issue if you remember was with the xpt files (the other formats were fine). And might be no longer happening because the dummy data has been changed and is now more polished

In the main, the issue might not appear for the dummy data, but when using the real data I got:

[js3110]

Works well for me! I tried with a complicated dataset :)

[m-kolomanski]

I don't understand this PR.

How am I supposed to test if it works if there is no data with the issue available?

I don't know what the expected result is. If the requirements are:

PPORRES = PPSTRESN = PPSTRESC

then we can just do:

mutate(PPORRES = 0, PPSTRESN = 0, PPSTRESC = 0)

and that will satisfy the requirements.

We seem to be fixing some kind of edge-case that shows up with specific numbers - but that is not reflected in the unit tests. If we discover some edge-case, we should cover it with tests, to make sure we don't break this with further updates. This is what the tests are for.

This is not a well prepared and described PR.

What is up with this weird syntax with column values reassignment and idx? If we want to skip NA values in formatting, why is if_else not used as with PPSTRESC?

Why are we so furiously converting character -> numeric -> character -> numeric? Let the numbers be numbers, derive formatted characters where needed.

I have tried a couple of examples from the attached screenshot and doing just

round(as.numeric(x), 8)

yields me the same results as

as.numeric(format(round(as.numeric(x[idx]), 8), scientific = FALSE))

so I don't get what is the reason? What is the edge case we are solving by doing this back and forth conversion?

This is some unorthodox code submitted with zero explanation on the reasoning behind those decisions.

[m-kolomanski]

BTW, I check it on main and dummy data and the numbers are different:

so considering:

issue is no longer in main

and

In the main, the issue might not appear for the dummy data

I am even more confused as to what the problem is, if it is not "numbers are different". Issue description has a total of 4 words, 1 brand name and 3 acronyms.

[Gero1999]

Hello @m-kolomanski! Your interpretation on the problem is correct. And you are right that it is still happening in main, so that is my bad, I may have not updated the env correctly when I checked.

The implementation seems to work (also in the xpt translated format), I just have some questions regarding the code part. Once clarified I can approve

[Shaakon35]

@js3110 @Gero1999 ready to review

[m-kolomanski]

Still there is no unit tests for that case we are solving here.

[Shaakon35]

@m-kolomanski Tests added.

@Gero1999 @js3110
There is still one issue that does not happen often but does not work:
if PPORRESU and PPSTRESU don't have the same unit, the rounding of the numeric and character columns is not exactly perfect, but I guess we can leave it like that for now.

[Shaakon35]

Review of PR #939 — fix PPSTRESN

Context

This PR addresses issue #561 where PPSTRESN != PPSTRESC causes Pinnacle 21 validation failures. The fix reformats the PPORRES, PPSTRESN, and PPSTRESC derivations in export_cdisc.R to ensure consistency and avoid scientific notation.

---

Issues

1. PPORRES type changed from character to numeric — breaks CDISC compliance

Current code (main):

PPORRES = as.character(round(as.numeric(PPORRES), 12)),

Proposed code:

PPORRES = if_else(!is.na(PPORRES),
                  round(suppressWarnings(as.numeric(PPORRES)), 12),
                  NA_real_),

PPORRES is a character variable in the CDISC PP domain ("Result or Finding in Original Units", Char type). The old code correctly kept it as character via as.character(). The new code returns NA_real_ / numeric, which:

• Violates the CDISC PP domain specification
• Will cause the downstream PPORRESU = ifelse(is.na(PPORRES), NA_character_, PPORRESU) check on line 383 to behave differently (comparing numeric NA vs character NA)
• May cause issues in xpt export since haven::write_xpt is type-sensitive

PPORRES should remain character. If the goal is consistency, format it the same way as PPSTRESC:

PPORRES = if_else(is.na(PPORRES),
                  NA_character_,
                  formatC(round(suppressWarnings(as.numeric(PPORRES)), 12),
                          format = "f", digits = 12, drop0trailing = TRUE) %>% trimws()),

2. suppressWarnings() silently hides data quality issues

If PPSTRES or PPORRES contains non-numeric values (e.g., "BLQ", "<LLOQ"), as.numeric() would produce a warning — which is useful signal. Suppressing it means bad data flows through silently as NA with no trace.

At minimum, consider logging a message when coercion produces NA on non-NA input, or document why suppression is acceptable here.

3. PPSTRESC should also be formatted consistently with PPORRES

If PPORRES (character) and PPSTRESC (character) should represent the same value in character form, they should use the same formatting logic. Currently they use different approaches.

4. Test coverage is narrow

The test uses a single value (0.000000123456789012345) to verify formatting. Consider adding:

• NA values — verify they propagate correctly
• Zero (0) — verify formatting
• Large values — verify no scientific notation for e.g. 123456789.123
• Negative values
• Values that are already character/non-numeric in the source data (if applicable)

The test also sets PPORRES = PPSTRES which doesn't test the case where they differ.

5. PR checklist is entirely unchecked

All checklist items (lintr, tests, docs, NEWS, version) are unchecked despite CI passing. The NEWS.md entry and version increment appear to be missing.

---

Minor observations

• The reordering of PPSTRESN before PPORRES is fine — PPSTRESN doesn't depend on PPORRES.
• Using if_else over ifelse is the right choice for type safety within dplyr::mutate.
• formatC(..., format = "f", digits = 12, drop0trailing = TRUE) %>% trimws() is a reasonable approach for non-scientific formatting.

---

Summary

The core idea (ensuring PPSTRESN and PPSTRESC are consistent and non-scientific) is correct and needed. However, the PPORRES type change from character to numeric is a regression against CDISC compliance and should be reverted. The suppressWarnings usage should be reconsidered or documented. Test coverage should be expanded to cover edge cases.

I agree with @m-kolomanski's earlier feedback that the PR would benefit from a clearer description of the specific edge case being fixed and more targeted test cases.

[js3110]

@m-kolomanski Tests added.

@Gero1999 @js3110 There is still one issue that does not happen often but does not work: if PPORRESU and PPSTRESU don't have the same unit, the rounding of the numeric and character columns is not exactly perfect, but I guess we can leave it like that for now.

this essentially means that any time the user changes the units the rounding wont be good- so I don't think this is something worth compromising on

[Shaakon35]

@m-kolomanski Tests added.
@Gero1999 @js3110 There is still one issue that does not happen often but does not work: if PPORRESU and PPSTRESU don't have the same unit, the rounding of the numeric and character columns is not exactly perfect, but I guess we can leave it like that for now.

this essentially means that any time the user changes the units the rounding wont be good- so I don't think this is something worth compromising on

Yeah I will try to find a solution, but it's hard :(