Skip to content

Hot fix: edits the trajectories to quantiles function #105

Merged
kaitejohnson merged 2 commits intomainfrom
hot-fix-missing-val-handling
Nov 26, 2025
Merged

Hot fix: edits the trajectories to quantiles function #105
kaitejohnson merged 2 commits intomainfrom
hot-fix-missing-val-handling

Conversation

@kaitejohnson
Copy link
Collaborator

@kaitejohnson kaitejohnson commented Nov 26, 2025

  • The missingness handling was by group, so if any of the draws had NAs (which will happen if one chain fails) the enture group would be excluded, hence, no quantiles could be produced. This issue arose in Thorigen on 2025-03-03 in one of the VM runs.

Summary by CodeRabbit

  • Bug Fixes
    • Improved data handling by enhancing the treatment of missing values in statistical calculations, resulting in more accurate and reliable quantile computations.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Nov 26, 2025

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

The R/utils.R file was modified to refactor the quantile calculation logic. The previous missing groups computation and anti-join step were removed, replaced with a direct NA filtering step and an added na.rm = TRUE parameter to the quantile function.

Changes

Cohort / File(s) Summary
Quantile calculation refactoring
R/utils.R
Removed missing_groups computation and anti_join operation; added NA filtering for value_col before quantile calculation; added na.rm = TRUE argument to stats::quantile call

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: fixing the trajectories-to-quantiles function's handling of missing values by changing from group-level exclusion to row-level filtering.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 02cbb0f and c73e168.

📒 Files selected for processing (1)
  • R/utils.R (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
  • GitHub Check: test-coverage
  • GitHub Check: macOS-latest (release)
  • GitHub Check: ubuntu-latest (release)
  • GitHub Check: windows-latest (release)
  • GitHub Check: lint-changed-files
🔇 Additional comments (1)
R/utils.R (1)

89-89: Filter logic correctly handles row-level NAs.

The filter step appropriately removes individual rows with NA values rather than excluding entire groups, which aligns with the PR objective to handle partial missingness within groups.

R/utils.R Outdated
Comment on lines 93 to 94
probs = !!quantiles,
nam.rm = TRUE
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Fix typo in parameter name.

Line 94 contains nam.rm = TRUE, but the correct parameter name for stats::quantile is na.rm, not nam.rm. This typo will cause the parameter to be unrecognised.

Apply this diff to correct the parameter name:

       !!quantile_value_name := stats::quantile(
         .data$value_col,
         probs = !!quantiles,
-        nam.rm = TRUE
+        na.rm = TRUE
       ),
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
probs = !!quantiles,
nam.rm = TRUE
probs = !!quantiles,
na.rm = TRUE
🤖 Prompt for AI Agents
In R/utils.R around lines 93 to 94, there is a typo in the quantile call: the
parameter is written as `nam.rm = TRUE` but the correct argument name is
`na.rm`. Replace `nam.rm` with `na.rm` so the `stats::quantile` call recognises
and applies the argument.

@kaitejohnson kaitejohnson merged commit 672a9b1 into main Nov 26, 2025
6 of 7 checks passed
@kaitejohnson kaitejohnson deleted the hot-fix-missing-val-handling branch November 26, 2025 12:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant