fix(turnover): parse timepoint out of tracer constants#9
Conversation
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
R/protein_turnover_ratio_helper.R (1)
150-157:⚠️ Potential issue | 🟡 MinorGuard against silent NA propagation when tracer names fail to normalize.
The fix correctly aligns
tracer_constantsnames with the parsedTimeValrepresentation. However, failure modes are silent:
- If any original name doesn't match
parse_timepoint's^[0-9]+pattern (e.g.,"30min","2h", typos), the normalized name becomes"NA", andtracer_constants[as.character(TimeVal)]returnsNA, makingH_fracsilentlyNA.- If
df_widecontains aTimeValnot present intracer_constants, the same silent-NAoutcome occurs.- Duplicate timepoints after normalization (e.g., both
"1hr"and"1h"→"1") are silently deduplicated by first match.Consider validating up front:
🛡️ Proposed validation
- - names(tracer_constants) = as.character(parse_timepoint(names(tracer_constants))) + + parsed_names <- parse_timepoint(names(tracer_constants)) + if (any(is.na(parsed_names))) { + stop("Could not parse tracer_constants names: ", + paste(names(tracer_constants)[is.na(parsed_names)], collapse = ", ")) + } + if (anyDuplicated(parsed_names)) { + stop("tracer_constants contains duplicate timepoints after normalization: ", + paste(parsed_names[duplicated(parsed_names)], collapse = ", ")) + } + names(tracer_constants) <- as.character(parsed_names) + + missing_tp <- setdiff(as.character(unique(df_wide$TimeVal)), names(tracer_constants)) + if (length(missing_tp) > 0) { + stop("tracer_constants is missing entries for timepoints: ", + paste(missing_tp, collapse = ", ")) + } df_wide <- df_wide %>% mutate( tracer_factor = tracer_constants[as.character(TimeVal)], H_frac = H_frac / tracer_factor )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@R/protein_turnover_ratio_helper.R` around lines 150 - 157, Validate parsed tracer names and lookup coverage before applying tracer factors: after setting names(tracer_constants) = as.character(parse_timepoint(names(tracer_constants))), check for any NA names returned by parse_timepoint and fail with a clear error listing offending original names; detect duplicated normalized names in tracer_constants and require the author to disambiguate (or aggregate) instead of silently keeping the first; before mutating df_wide, ensure all unique df_wide$TimeVal (as.character(parse_timepoint(...)) if needed) are present in names(tracer_constants) and throw an error or warning listing missing TimeVal entries so tracer_factor lookup (tracer_constants[as.character(TimeVal)]) cannot silently produce NA for H_frac.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@R/protein_turnover_ratio_helper.R`:
- Around line 150-157: Validate parsed tracer names and lookup coverage before
applying tracer factors: after setting names(tracer_constants) =
as.character(parse_timepoint(names(tracer_constants))), check for any NA names
returned by parse_timepoint and fail with a clear error listing offending
original names; detect duplicated normalized names in tracer_constants and
require the author to disambiguate (or aggregate) instead of silently keeping
the first; before mutating df_wide, ensure all unique df_wide$TimeVal
(as.character(parse_timepoint(...)) if needed) are present in
names(tracer_constants) and throw an error or warning listing missing TimeVal
entries so tracer_factor lookup (tracer_constants[as.character(TimeVal)]) cannot
silently produce NA for H_frac.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c5a852b9-ae15-42dd-91d5-4286b3bbbf94
📒 Files selected for processing (1)
R/protein_turnover_ratio_helper.R
Motivation and Context
The
calculateTurnoverRatios()function supports tracer normalization via thenormalize_tracerparameter, which uses a named numeric vector (tracer_constants) to normalize heavy isotope fractions. When normalizing tracer incorporation, the code needs to look up tracer constant values for each timepoint. However, a mismatch existed between how timepoint names were formatted in thetracer_constantsparameter versus how they were parsed in the data.Specifically:
tracer_constantswith names like"0hr","1hr","12hrs","168hrs"parse_timepoint()to convert them to numeric hours (0, 1, 12, 168)tracer_constants[as.character(TimeVal)]converts TimeVal to character strings like"0","1","12","168"Solution: Parse the names of
tracer_constantsthroughparse_timepoint()and convert the result back to character strings, ensuring the constant names use the same numeric-only format as the parsed TimeVal values.Changes
names(tracer_constants) = as.character(parse_timepoint(names(tracer_constants)))tracer_constantsin the tracer normalization blocktracer_constantsvector to match the parsed timepoint format (numeric hours as character strings)tracer_constants[as.character(TimeVal)]finds matching keysUnit Tests
No unit tests were added or modified in this PR. The function currently has no existing test coverage for the tracer normalization feature.
Coding Guidelines
No coding guideline violations identified. The change follows the existing code style and conventions used throughout the file.