fix: fail loud on non-logical half-life point columns and annotate unreportable half-life - #591
Open
billdenney wants to merge 2 commits into
Open
fix: fail loud on non-logical half-life point columns and annotate unreportable half-life#591billdenney wants to merge 2 commits into
billdenney wants to merge 2 commits into
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>
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