fix: fail loud on non-logical half-life point columns and annotate unreportable half-life - #591
Merged
Merged
Conversation
…reportable half-life Two silent-failure modes around half-life calculation are fixed for issue 583: - A non-logical exclude_half.life or include_half.life column (e.g. character "yes") was accepted silently and excluded or included nothing, because the per-point columns are matched against logical TRUE. PKNCAconc() now errors at construction, naming the column and stating that a logical (TRUE/FALSE/NA) column is required, and pk.nca() re-validates at calculation time in case the column was modified after construction. - When no candidate lambda.z window survived point selection (e.g. a well-fitting window with lambda.z <= 0 anchoring the adjusted r-squared tolerance), pk.calc.half.life() returned NA with no exclusion reason. It now attaches "No valid terminal phase: ..." via the established attr(result, "exclude") mechanism for both the log-linear and Tobit methods, so the reason lands in the exclude column of pk.nca() results. Normal fits and the existing too-few-points messaging are unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Passing a column name to PKNCAconc(exclude_half.life=) or PKNCAconc(include_half.life=) that does not exist in the data silently created an all-NA logical column via setAttributeColumn()'s default-value path, so a typo'd name deactivated the point selection (an all-NA column is treated as "not in use") and passed the is.logical() validation added earlier for issue 583. PKNCAconc() now errors at construction using setAttributeColumn()'s stop_if_default hook, naming the argument and the missing column. The hook fires only on the missing-column path, so non-character input keeps its existing "col_name must be a character scalar." error and existing columns (including all-NA logical ones) are unaffected. Other setAttributeColumn() callers are unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
billdenney
commented
Aug 20, 2026
| #' unless it is entirely `NA` (so an all-`FALSE` column still counts as in | ||
| #' use), so leave it `NA` (rather than `FALSE`) where the mechanism should not | ||
| #' apply. Only one of `exclude_half.life` and `include_half.life` may be in | ||
| #' apply. A non-logical column (e.g. character `"yes"`) or a column name that |
Member
Author
There was a problem hiding this comment.
Remove the extra explanation here. Explaining what is allowed and the error is sufficient.
| # lambda.z <= 0 anchors the adjusted r-squared tolerance so that no | ||
| # window with lambda.z > 0 is within the tolerance. | ||
| attr(ret, "exclude") <- | ||
| "No valid terminal phase: no window with lambda.z > 0 within the adjusted r-squared tolerance of the best fit" |
Member
Author
There was a problem hiding this comment.
Rename "window" to "span" as it is the typical description for the calculation window.
| } else { | ||
| # No window with a positive elimination rate (or no converged fit) | ||
| attr(ret, "exclude") <- | ||
| "No valid terminal phase: no Tobit window with lambda.z > 0" |
Member
Author
There was a problem hiding this comment.
Rename "window" to "span" as it is the typical description for the calculation window.
| } | ||
| uses_include_hl <- FALSE | ||
| if ("include_half.life" %in% names(conc_data_interval)) { | ||
| # Guards against the column being modified after PKNCAconc() validation; |
Member
Author
There was a problem hiding this comment.
Add this test to assert_PKNCAconc(), and validate it there. If necessary, add an assert_PKNCAconc() to the beginning of this function.
| } | ||
| uses_exclude_hl <- FALSE | ||
| if ("exclude_half.life" %in% names(conc_data_interval)) { | ||
| if (!is.logical(conc_data_interval$exclude_half.life)) { |
Member
Author
There was a problem hiding this comment.
Add this test to assert_PKNCAconc(), and validate it there. If necessary, add an assert_PKNCAconc() to the beginning of this function.
…n-failloud # Conflicts: # NEWS.md
Addresses two review comments and simplifies the rest. The exclusion messages and the comments around them now say "span" rather than "window", matching the usual description of the lambda z calculation span, along with the tests that pin those messages. Nine pre-existing uses of "window" in the Tobit roxygen are left alone as unrelated. The exclude_half.life/include_half.life documentation drops the example of a rejected value and states the rule: the column must be logical and must exist in the data. Three comments narrated the bug's history rather than the code -- character columns "were previously accepted silently", a missing column name "previously created an all-NA logical column silently", and the guard in pk.nca() explaining what would "otherwise silently" happen. Each now states what the code or assertion does. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Addresses the review comments on pk.calc.all.R. The logical-column check existed twice: once inline in PKNCAconc() and once inline in pk.nca.interval(), with different messages for the same problem. It now lives only in assert_PKNCAconc(), which PKNCAconc() calls before returning and assert_PKNCAdata() calls on the concentration object, so pk.nca() reaches it without a check of its own. pk.nca.interval() operates on a plain data.frame slice rather than a PKNCAconc, so it could not call assert_PKNCAconc() directly; routing through assert_PKNCAdata() covers the case the inline check was there for -- a column replaced after construction. The surviving message names the offending column, which the pk.nca.interval() one did not, so the two tests that pinned the shorter text now expect the column name. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #583.
(1) Non-logical
exclude_half.life/include_half.lifecolumns (e.g."yes", which silently excluded nothing) now error with an informative message at bothPKNCAconc()construction and again atpk.nca()time (catching post-construction mutation). (2) When no λz window survives the selection gate — including the case where a well-fitting λz ≤ 0 window anchors the adjusted-R² tolerance — the NA half-life now carries an exclusion reason via the standardattr(result, "exclude")mechanism, in both the log-linear and Tobit branches. Normal fits, the too-few-points path, and manual selection are unaffected.Verification: full suite 2,717 pass / 0 fail (the 2 warnings are pre-existing on baseline); new tests pin the exact error messages, the new exclusion reasons (direct call and through
pk.nca()), and clean exclude columns for healthy fits.Note: a typo'd column name passed to these arguments still silently creates an all-NA "not in use" column — a separate silent path possibly worth its own issue.
🤖 Generated with Claude Code