Skip to content

Fix SCRIB bugs - #1190

Open
lehendo wants to merge 6 commits into
sunlabuiuc:masterfrom
lehendo:favscrib
Open

Fix SCRIB bugs#1190
lehendo wants to merge 6 commits into
sunlabuiuc:masterfrom
lehendo:favscrib

Conversation

@lehendo

@lehendo lehendo commented Aug 17, 2026

Copy link
Copy Markdown
Collaborator
  • Both SCRIB bug fixes (unsquared ambiguity term, fill_max applied at inference)
  • tests/core/test_scrib.py (11 tests) covering fill_max resolution, the empty-set fix (proven via forced-threshold stress test), and the loss formula
  • Docs updated with the fill_max consistency note
  • CI gate passes
  • All 11 tests pass on real hardware

The overall-risk (float risk) loss squared the chance-ambiguity term,
contradicting the paper's Eq. 2 and Algorithm 2 (both specify it
unsquared) and inconsistent with the already-correct class-specific
loss in the same file. Fixed in both the pure-Python and Cython
implementations.

fill_max (default True) was applied during threshold search but never
at inference: forward() could return empty prediction sets despite the
documented promise to fill them with the max-predicted class. forward()
now resolves and applies the same fill_max behavior used during
calibration.
Adds tests/core/test_scrib.py covering: fill_max resolution (including
the loss_kwargs-without-fill_max-key case), forward() correctly filling
empty prediction sets with the argmax class when fill_max=True (and
correctly leaving them empty when False), and the unsquared
chance-ambiguity term in the overall-risk loss.

Documents the fill_max consistency between calibrate() and forward() in
docs/api/calib/pyhealth.calib.predictionset.rst.
Confirms the "-1" in the violation-control (delta mode) cutoff isn't
an open question -- it's the paper's own formula. The paper's Appendix
B.3, Algorithm 5 explicitly specifies q = ((N+1)*delta - 1) /
tree.root.sum, which matches this code exactly (the /tree.root.sum is
algebraically absorbed since query_cumu_weight takes an absolute
cumulative weight, not a normalized quantile). Independently re-derived
from the Tc,delta proof (Theorem 4.6, Appendix A.4) to rule out a typo
in the paper itself: negating {Ti} and adding an infinity placeholder
for the unseen test point reduces to a direct forward quantile query
on the raw Ti values with target cutoff delta*(N+1) - 1 -- the same
finite-sample correction pyhealth's own base_conformal._query_quantile
already uses via ceil((1-alpha)(N+1)).

Replaces the original author's in-code "Should we minus 1??" comment,
which was leftover design-time scratch notes that predated their own
paper's final derivation, not a live open question.
core.FavMac had no docstring at all -- add one describing its role
(the abstract engine behind the public-facing
pyhealth.calib.predictionset.FavMac) and a verified, runnable example
via the concrete FavMac_GreedyRatio subclass.
The example demonstrates LABEL and CovariateLabel but never mentions
SCRIB as an alternative for class-specific risk control. Add a pointer
to it in the module docstring.
@lehendo
lehendo requested a review from jhnwu3 August 18, 2026 01:14
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