OA-only mode: remove dragen_hrd dependency, keep it optional not deleted (sash #56) - #102
Open
qclayssen wants to merge 4 commits into
Open
OA-only mode: remove dragen_hrd dependency, keep it optional not deleted (sash #56)#102qclayssen wants to merge 4 commits into
qclayssen wants to merge 4 commits into
Conversation
Regression guard: 563f946 accidentally re-introduced required=TRUE, causing crashes on samples without DRAGEN HRD files.
Remove --dragen_hrd from canrep CLI, cancer_rmd() function, Rmd params, and render.R. The dragen_hrd() and hrd_results_tabs() utilities in umccrise.R are kept as exported functions. The HRD section now shows CHORD and HRDetect only; dragen_hrd_res is always constructed with NULL input so hrd_results_tabs still renders (DRAGEN column shows NA).
The oa-only branch removed DRAGEN HRD support from cancer_report.Rmd entirely, but the report is a single shared template used by every umccrise/sash cancer report — this silently dropped DRAGEN scoring for regular DRAGEN-capable runs too, not just OA-only ones. It also left the report rendering a "DRAGEN" section always filled with NA instead of omitting it, since hrd_results_tabs() was never updated. - Restore dragen_hrd as an optional parameter end-to-end (cancer_rmd(), --dragen_hrd CLI arg, render.R params) — was already optional before the removal, so this is a revert, not new API. - hrd_results_tabs() now takes dragen_res = NULL and genuinely omits the DRAGEN column/spanner when absent, instead of showing it blank. Also fixes a stray left-border artifact that appeared on the CHORD column when it became the leftmost group (was only ever tested with DRAGEN present). - cancer_report.Rmd: DRAGEN description block, HRD summary row, and the hrd/*-dragen output artifact are all now conditional on whether DRAGEN was actually supplied. - pcgr_format_categories() (PCGR skip/selection status parsing, sash #52): fixed a no-separator string join that could fuse adjacent categories together if bolt emits pcgr_selection_filters as a multi-element vector, and added defensive validation so a malformed category string is skipped instead of aborting the whole render. Nested nested nested ifelse() chains collapsed to case_when+switch so pcgr_value_fragment/pcgr_details can't drift out of sync. - Added test coverage for hrd_results_tabs() (both branches) and pcgr_format_categories() (previously untested). Reviewed via 2x 8-angle multi-agent review (medium effort) covering correctness, removed-behavior, cross-file impact, reuse, simplification, efficiency, altitude, and conventions — all confirmed findings fixed, verified findings addressed inline below. Known, deliberate gaps (not fixed here, flagged for follow-up): - The cli_warn previously fired on any missing --dragen_hrd is not restored, since it would now be spurious for every legitimate OA-only run (no way to distinguish "intentional OA-only" from "accidental omission" at this layer). - hrd_results_tabs()'s dragen_res = NULL couples a general-purpose exported function to one caller's data-availability needs; a cleaner design would push that decision into the calling Rmd. Left as-is since re-architecting is bigger scope than this regression fix. - dplyr::select() on a malformed (but non-NULL) dragen_res still throws an opaque tidyselect error — pre-existing behavior, unchanged by this diff.
cancer_report.Rmd called the unqualified %||% operator, but it's neither imported from rlang (NAMESPACE only imports := and .data) nor available as a base R builtin before R 4.4.0 — and this package declares Depends: R (>= 4.1). On R 4.1-4.3 without rlang attached, this threw "could not find function %||%" on the PCGR-selection-filtering display path (sash #52). Fix by namespace-qualifying the call as rlang::`%||%`(...), so it resolves regardless of R version or attached packages, with no NAMESPACE change needed. Also extract pcgr_format_categories() out of the Rmd chunk into R/umccrise.R as an exported function, since it was previously duplicated verbatim into tests/testthat/test-pcgr-status.R just to get coverage — that test file now exercises the real function, and a roxytest @TestExamples block covers it too.
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
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.
Summary
Supports sash's OA-only mode (driven by the Grimmond Lab SBX evaluation — SBX is Roche's sequencer, samples arrive with no DRAGEN output) by making DRAGEN HRD scoring genuinely optional in the cancer report, rather than removed outright.
The first commit on this branch (
d012f30) removeddragen_hrdfrom the report entirely. Sincecancer_report.Rmdis the single shared template behind gpgr'scanrepCLI — used for every report gpgr generates, not just OA-only ones — that would have silently dropped DRAGEN HRD scoring from regular, DRAGEN-capable sash runs too. It also left the report rendering a "DRAGEN" section always filled with NA instead of omitting it.This commit fixes both:
dragen_hrdas an optional parameter end-to-end (cancer_rmd(),--dragen_hrdCLI arg,render.R) — it was already optional pre-removal, so this is a revert, not new API surface.hrd_results_tabs()now takesdragen_res = NULLand genuinely omits the DRAGEN column/spanner when absent, instead of showing it blank. Also fixes a stray left-border artifact that appeared on the CHORD column once it became the leftmost group (caught during review — the function had only ever been exercised with DRAGEN present).hrd/*-dragenoutput artifact are all now conditional on whether DRAGEN was actually supplied.pcgr_format_categories()(PCGR skip/selection status parsing, sash umccrise canrep: flag if sample is hypermutated #52): fixed a no-separator string join that could fuse adjacent categories together if bolt emitspcgr_selection_filtersas a multi-element vector, added defensive validation so a malformed category string is skipped instead of aborting the whole render, and collapsed nestedifelse()chains tocase_when+switchso the value/details strings can't drift out of sync.hrd_results_tabs()(both branches) andpcgr_format_categories()— both were previously untested.Depends on: sash #58, bolt #34 (companion PRs for the OA-only path itself — this PR only touches the cancer report layer).
Known, deliberate gaps (flagged for follow-up, not blocking)
cli_warnpreviously fired on any missing--dragen_hrdis not restored — it would now be spurious on every legitimate OA-only run, and there's no way to distinguish "intentional OA-only" from "accidental omission" at this CLI layer.hrd_results_tabs()'sdragen_res = NULLcouples a general-purpose exported function to one caller's data-availability needs; a cleaner design would push that decision into the calling Rmd. Left as-is since re-architecting is bigger scope than this regression fix.dplyr::select()on a malformed (but non-NULL)dragen_resstill throws an opaque tidyselect error — pre-existing behavior, confirmed unchanged by this diff.Test plan
devtools::test())test-hrd-results-tabs.R(DRAGEN present/absent, both render to HTML without error),test-pcgr-status.R(category parsing, malformed-input handling, multi-element vector join)hrd_results_tabs()output shape andgt::as_raw_html()rendering for both branches