Open
Conversation
Adds design/ directory (excluded from R package build via .Rbuildignore) containing architecture review, best practices audit, performance analysis, code clarity notes, future work suggestions, and potential bugs identified during a thorough review of the PKNCA codebase. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add comments to aucintegrate_log() and aumcintegrate_log() explaining that equal concentrations cannot reach these functions because choose_interval_method() only assigns "log" to strictly declining intervals with no zero endpoints. Mark bug #1 as resolved in the design review document. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add comment to interpolate_conc_log() explaining that zero/negative concentrations cannot reach it because choose_interval_method() assigns "zero" or "linear" to any interval with a zero endpoint. Mark bug #2 as resolved in the design review document. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add an explicit check that idx_tlast has exactly one element after which(time == tlast), placed after the all-zeros early return (where tlast is NA). Raises an informative error if tlast is absent (e.g. due to floating point inequality) or duplicated. Add test using the classic 0.1 + 0.2 != 0.3 floating point case to confirm the error fires correctly. Mark bug #3 resolved. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add comment explaining that when min.hl.points == 2 with only 2 points, setting mask_best to all positive-lambda.z fits is intentional — the existing sum(mask_best) > 1 block immediately below resolves ties by selecting the fit with the most points. Mark bug #4 as resolved in the design review document. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…lve bug #5 Add comment explaining that tlast = tfirst + 1 is an intentional sentinel used only for comparisons against ret$time, and is guaranteed to exceed all observed time values since tfirst = max(ret$time). Mark bug #5 as resolved in the design review document. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…lve bugs #6 and #7 Replace all single | and & operators with || and && in scalar if() conditions across 24 R source files for idiomatic short-circuit evaluation. Vectorized uses of | and & (inside all(), any(), or column-level mask construction) are unchanged. Add comments to auc_integrate.R and cleaners.R documenting that %in% 0 for BLQ detection is intentionally exact equality, since BLQ values are cleaned to exactly 0 upstream and a tolerance cannot be used without domain knowledge of what a "low" concentration means. All 2292 tests pass. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
assert_conc_time() enforces unique time points before any interpolation logic runs, so data\$time cannot contain duplicates at the call site in interpolate.conc(). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ntentional stopifnot(!duplicated(x)) is idiomatic R: stopifnot() accepts a vector and stops if any element is FALSE, so wrapping in any() is redundant. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ve issues 10-11 Issue 10: pk.calc.auc.inf.obs with lambda.z=NA propagates NA through aucintegrate_inf (clast/NA) and correctly returns NA_real_. Test added to tests/testthat/test-auc.R to confirm this behavior. Issue 11: getGroups.PKNCAconc() lazy default argument is safe because PKNCAconc objects are immutable in normal use. No code change needed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
All 11 items have been resolved (fixes applied, false positives documented, or confirmed as intended behavior). Co-Authored-By: Claude Sonnet 4.6 <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.
No description provided.