feat: Phase D5 explicit cohort subgroup validation, head-covered trial bindings, and clustered continuous bootstrap - #64
Conversation
…l bindings, and clustered continuous bootstrap
|
Droid finished @txmed82's task —— View job The Phase D5 changes look coherent and well-tested, especially the crash-safe trial binding persistence and manifest-backed subgroup analysis. Fix the subgroup key normalization overwrite and harden Markdown rendering against subgroup value injection so reports stay trustworthy. |
![factory-droid[bot]](https://avatars.githubusercontent.com/in/358017?s=60&v=4){kind=small}
| f"subgroup key {k!r} is a reserved identifier; patient/site/case/episode IDs " | ||
| "must be specified in their dedicated fields, not subgroups" | ||
| ) | ||
| validated[k] = v |
There was a problem hiding this comment.
[P1] Reject normalized subgroup key collisions
validate_subgroups() strips keys and then writes validated[k] = v without checking whether the normalized key k was already seen, so inputs like {" age": "adult", "age": "peds"} silently overwrite an axis and change downstream subgroup analysis. Detect collisions after normalization and raise TaskContractError to fail fast.
| validated[k] = v | |
| if k in validated: | |
| raise TaskContractError( | |
| f"subgroup key {k!r} appears multiple times after normalization" | |
| ) | |
| validated[k] = v |
| pwr = "underpowered (<10)" if sg["is_underpowered"] else "adequate" | ||
| lines.append( | ||
| f"| {sg['subgroup']} | {sg['count']} | {sg['rate']:.4f} | {ci_str} | {pwr} |" | ||
| f"| {sg['axis']} | {sg['subgroup']} | {sg['count']} | " |
There was a problem hiding this comment.
[P1] [security] Escape subgroup values in Markdown table output
render_markdown() interpolates sg['subgroup'] (from split-manifest subgroups, validated only as a non-empty string) directly into a pipe-delimited Markdown table and an inline-code snippet, so values containing |, backticks, or newlines can corrupt/spoof the rendered report (and in some Markdown renderers, enable HTML/Markdown injection). Escape table cells (at least | and newlines) and sanitize backticks before writing scorecard.md.
…uster rejection, and unassessed row handling
|
@droid review |
|
Droid finished @txmed82's task —— View job I validated three high-signal comments: one correctness bug that can crash non-manifest runs, plus two statistical/contract issues around clustered bootstrap where the reported mean can mismatch the CI and invalid parameters can currently raise runtime errors. |
| patient_id=m_entry.patient_id or "" if m_entry else "", | ||
| site_id=m_entry.site_id or "" if m_entry else "", |
There was a problem hiding this comment.
[P0] Fix manifest binding conditional precedence
In both _run_predictions and _run_interactive, patient_id=m_entry.patient_id or "" if m_entry else "" (and site_id) evaluates m_entry.patient_id before the conditional due to operator precedence, so runs without a split manifest (m_entry is None) crash with AttributeError; parenthesize the conditional expression (and apply the same fix in both blocks).
| patient_id=m_entry.patient_id or "" if m_entry else "", | |
| site_id=m_entry.site_id or "" if m_entry else "", | |
| patient_id=(m_entry.patient_id or "") if m_entry else "", | |
| site_id=(m_entry.site_id or "") if m_entry else "", |
| ci_method = "bootstrap" | ||
| if len(clusters) >= 2: | ||
| ci_95 = list(clustered_bootstrap_mean_ci(clusters)) | ||
| ci_method = "clustered_bootstrap" | ||
| else: | ||
| ci_95 = list(bootstrap_mean_ci(numeric)) if numeric else None | ||
| row.update( | ||
| { | ||
| "mean": fmean(numeric) if numeric else None, | ||
| "min": min(numeric) if numeric else None, | ||
| "max": max(numeric) if numeric else None, | ||
| "ci_95": ci_95, | ||
| "ci_method": ci_method, | ||
| "confidence": 0.95, | ||
| "draws": 1000, | ||
| } | ||
| ) |
There was a problem hiding this comment.
[P1] Align continuous mean with clustered CI estimand
When len(clusters) >= 2, clustered_bootstrap_mean_ci() returns a CI for the equal-weighted mean of per-cluster means, but the scorecard still reports "mean": fmean(numeric) (observation-weighted), so the point estimate and CI target different estimands for uneven cluster sizes; compute and report the cluster-mean when using the clustered CI so the interval matches the displayed mean.
| ci_method = "bootstrap" | |
| if len(clusters) >= 2: | |
| ci_95 = list(clustered_bootstrap_mean_ci(clusters)) | |
| ci_method = "clustered_bootstrap" | |
| else: | |
| ci_95 = list(bootstrap_mean_ci(numeric)) if numeric else None | |
| row.update( | |
| { | |
| "mean": fmean(numeric) if numeric else None, | |
| "min": min(numeric) if numeric else None, | |
| "max": max(numeric) if numeric else None, | |
| "ci_95": ci_95, | |
| "ci_method": ci_method, | |
| "confidence": 0.95, | |
| "draws": 1000, | |
| } | |
| ) | |
| mean = fmean(numeric) if numeric else None | |
| ci_method = "bootstrap" | |
| if len(clusters) >= 2: | |
| ci_95 = list(clustered_bootstrap_mean_ci(clusters)) | |
| ci_method = "clustered_bootstrap" | |
| mean = fmean([sum(vals) / len(vals) for vals in clusters.values()]) | |
| else: | |
| ci_95 = list(bootstrap_mean_ci(numeric)) if numeric else None | |
| row.update( | |
| { | |
| "mean": mean, | |
| "min": min(numeric) if numeric else None, | |
| "max": max(numeric) if numeric else None, | |
| "ci_95": ci_95, | |
| "ci_method": ci_method, | |
| "confidence": 0.95, | |
| "draws": 1000, | |
| } | |
| ) |
| if not clusters: | ||
| raise TaskContractError("clustered bootstrap needs at least one cluster") | ||
| cluster_keys = sorted(clusters.keys()) | ||
| k = len(cluster_keys) | ||
| for key, vals in clusters.items(): |
There was a problem hiding this comment.
[P1] Validate confidence and draws in clustered bootstrap
clustered_bootstrap_mean_ci() validates clusters contents but never checks confidence or draws like bootstrap_mean_ci() does, so draws < 1 crashes with IndexError when indexing percentiles and out-of-range confidence can produce invalid indices and wrong bounds; add the same contract checks here.
| if not clusters: | |
| raise TaskContractError("clustered bootstrap needs at least one cluster") | |
| cluster_keys = sorted(clusters.keys()) | |
| k = len(cluster_keys) | |
| for key, vals in clusters.items(): | |
| if not clusters: | |
| raise TaskContractError("clustered bootstrap needs at least one cluster") | |
| if not 0.0 < confidence < 1.0: | |
| raise TaskContractError(f"confidence {confidence!r} must be in (0, 1)") | |
| if draws < 1: | |
| raise TaskContractError(f"draws {draws!r} must be >= 1") | |
| for key, vals in clusters.items(): | |
| if not vals: | |
| raise TaskContractError( | |
| f"cluster {key!r} is empty; clustered bootstrap requires non-empty observations" | |
| ) |
…rcement, and continuous subgroup estimate label
|
@droid review |
|
Droid finished @txmed82's task —— View job The D5 manifest-backed binding/subgroup/CI work looks cohesive and well tested. The main remaining issues are runner-level contract gaps where independent_case_unit is not consistently persisted or used to compute independent_cases, which can make reported degrees of freedom and downstream uncertainty outputs inconsistent. |
…rify n=2 patient discrimination
|
@droid review |
|
Droid finished @txmed82's task —— View job I validated 1 candidate comment and approved it. It flags a real correctness issue where the common independent_case_unit alias "center" is treated as site-disjoint in cartesian but not normalized in run_job, which can silently miscount independent cases and affect downstream uncertainty/clustering. |
…-loop Cartesian persistence
|
@droid review |
| independent_cases=None, | ||
| split_manifest_digest="", | ||
| split="", |
There was a problem hiding this comment.
[P1] Persist independent_case_unit in closed-loop results
run_job threads independent_case_unit into _run_closed_loop, but the assemble_job_result(...) call drops it, so closed-loop result.json always has independent_case_unit=="" even when the config recorded a normalized unit. That breaks consistency with the other runners and prevents downstream reporting (for example, clustered uncertainty) from knowing the intended independent unit on closed-loop runs.
| independent_cases=None, | |
| split_manifest_digest="", | |
| split="", | |
| independent_cases=None, | |
| split_manifest_digest="", | |
| split=split or "", | |
| independent_case_unit=independent_case_unit, |
| evaluated_ids, unit=count_unit | ||
| ) | ||
| return assemble_job_result( | ||
| task=task, |
There was a problem hiding this comment.
[P1] Count independent_cases using independent_case_unit
In _run_predictions and _run_interactive, independent_case_unit is now recorded on the JobResult, but independent_cases is still computed with unit="case" unconditionally when a split manifest is present. If a stage declares patient- or site-level independence, this will misreport the independent case count (degrees of freedom) and can diverge from the unit used for clustered resampling; compute the count with unit=independent_case_unit when it is one of {"patient","site","case"}.
| u = independent_case_unit.strip().lower() | ||
| if u in ("patient", "patient_id"): | ||
| normalized_unit = "patient" | ||
| elif u in ("site", "site_id"): |
There was a problem hiding this comment.
[P1] Normalize independent_case_unit alias "center" to "site"
cartesian._independent_case_count() treats stage.independent_case_unit="center" as site-disjoint, but run_job() normalization does not, so dataset-backed runs using that alias will record independent_case_unit="center" and compute independent_cases as if it were case-disjoint. This silently produces incorrect independent-case counts and downstream uncertainty/clustering behavior; normalize "center" to "site" alongside the existing site aliases.
| elif u in ("site", "site_id"): | |
| elif u in ("site", "site_id", "center"): | |
| normalized_unit = "site" |
| ) | ||
| clusters[unit_val].append(float(m.value)) | ||
|
|
||
| if len(clusters) >= 2: |
There was a problem hiding this comment.
[P1] Do not fall back to i.i.d. bootstrap with a declared cluster unit
When independent_case_unit maps to patient/site/case, scorecard_data() uses clustered bootstrap only for len(clusters) >= 2 and otherwise falls back to i.i.d. bootstrap_mean_ci(numeric). If all assessed trials belong to a single cluster, this treats non-independent observations as independent and produces a CI that contradicts the declared independent-unit contract; use clustered_bootstrap_mean_ci() whenever clustering is declared and clusters is non-empty (it already handles k==1).
| if len(clusters) >= 2: | |
| if clusters: | |
| ci_95 = list(clustered_bootstrap_mean_ci(clusters, seed=0)) | |
| ci_method = "clustered_bootstrap" | |
| cluster_level_means = [fmean(v) for v in clusters.values() if v] | |
| mean_val = fmean(cluster_level_means) if cluster_level_means else None | |
| else: | |
| ci_95 = list(bootstrap_mean_ci(numeric, seed=0)) if numeric else None | |
| ci_method = "bootstrap" | |
| mean_val = fmean(numeric) if numeric else None |
2 participants
What: Persists TrialBinding atomically before result.json commit; reconstructs case/patient/site/subgroups into TrialRecord on resume; enforces shared validate_subgroups with reserved-key and slug constraints; derives subgroups strictly from explicit cohort fields rather than exposing patient IDs; activates clustered bootstrap over patient clusters; renders confidence intervals on leaderboard.
Why: Fully resolve D5 acceptance by guaranteeing head-covered trial metadata persistence across crash recovery, protecting clinical privacy in subgroup analysis, and estimating continuous metric uncertainty with clustered resampling.
Verify: Full test suite green (1582 passed, 10 skipped); 12 tests in tests/test_uncertainty.py including crash recovery, corrupt binding, and benchmark report fixtures; ruff + format + mypy strict clean.