`pk.calc.time_above()` with lin-up/log-down silently drops segments that end at a negative value
Maintainers usually reply within 1 day
Nobody has claimed this yet.
Assessment
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Newbie friendliness
- 79/100
Research direction
Start by locating pk.calc.time_above() and its lin-up/log-down segment classification; compare that logic with the AUC calculation's linear fallback. Add tests for the two negative-value examples, unchanged all-positive results, and a segment ending at zero. Done when every crossing segment is counted once and the reported totals match the examples.
Written by the indexing model from the issue text.
Description
Summary
In the lin-up/log-down branch, a segment that crosses from above to below the threshold is classified as linear_down only when conc2 == 0, and as log_down only when conc2 > 0. When conc2 < 0 it is neither, so the segment adds nothing to the total.
This matters for PD data: change from baseline, or effects with negative values. The only signal is the generic "Negative concentrations found" warning. A negative threshold over all-negative data returns 0.
Reprex
library(PKNCA)
# Exact (linear interpolation): 1/3 + 3/5 + 3/4 = 1.6833
suppressWarnings(pk.calc.time_above(conc = c(1, -2, 3, -1), time = 0:3, conc_above = 0, method = "linear"))
#> 1.683333
suppressWarnings(pk.calc.time_above(conc = c(1, -2, 3, -1), time = 0:3, conc_above = 0, method = "lin up/log down"))
#> 0.6 # only the rising segment is counted
# Negative threshold, both values negative; exact 0.5
suppressWarnings(pk.calc.time_above(conc = c(-1, -3), time = 0:1, conc_above = -2, method = "lin up/log down"))
#> 0
Proposed fix
Use log interpolation only where it is defined, conc1 > conc2 > 0, and linear interpolation for every other falling segment. That matches how the AUC calculation falls back to linear:
log_down <- conc2 > 0 & conc1 > 0 & conc2 < conc1
linear_down <- !log_down
Then apply mask_first & linear_down and mask_first & log_down, so every crossing segment falls into exactly one branch. An internal check that each mask_first and mask_second segment is counted once would catch regressions of this kind.
Tests to add
- The two examples above under lin-up/log-down: 1.6833 and 0.5.
- Results for all-positive data are unchanged.
- A segment ending exactly at 0 still uses linear interpolation.
- Dominant language
- R
- Stars
- 86
- Forks
- 32
- Avg merge
- 14h 17m
- Merged PRs (30d)
- 50
Getting set up
- No Dockerfile or Docker Compose file
- No pull request template
- Read the contributing guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
More from humanpred/pknca
-
Difficulty 4/5 3-5 days Newbie friendliness 45/100
Maintainers usually reply within 1 day
-
Difficulty 3/5 1-2 days Newbie friendliness 67/100
Maintainers usually reply within 1 day
-
Difficulty 4/5 3-5 days Newbie friendliness 58/100
Maintainers usually reply within 1 day
-
Feature request: SDTM input functionsPossibly taken @Gero1999 claimed this 227 days ago. Open
Difficulty 4/5 3-5 days Newbie friendliness 20/100
humanpred/pknca#516 · 2 comments ·
Maintainers usually reply within 1 day
-
Join COSAOpen
Difficulty 5/5 Over a week Newbie friendliness 10/100
humanpred/pknca#393 · 2 comments ·
Maintainers usually reply within 1 day
Similar issues
-
Difficulty 2/5 1-3 hours Newbie friendliness 68/100
-
Difficulty 2/5 1-3 hours Newbie friendliness 71/100
-
Difficulty 2/5 1-3 hours Newbie friendliness 72/100
-
Difficulty 2/5 1-3 hours Newbie friendliness 76/100
OHDSI/PatientLevelPrediction#688 ·
-
Difficulty 2/5 1-3 hours Newbie friendliness 78/100
rfordatascience/tidytuesday#1098 ·