Release v0.3.0: drift-detection features, calibration metrics, and bug fixes - #771
Merged
Conversation
- errorfill(): matplotlib removed ax._get_lines.prop_cycler; use get_next_color() instead, which was crashing any plot call with color=None. - TSTester.test_shift(): stop mutating p_val_threshold in place on every call (Bonferroni correction was compounding across repeated calls in Detector's loops); also guard against UnboundLocalError when X_t isn't a plain ndarray. - ContextMMDWrapper: was missing preprocess_at_init in its positional arg list to alibi-detect's ContextMMDDrift, silently shifting every later argument by one slot (the reason the ctx_mmd test path was skipped as broken). Converted both ContextMMDWrapper and LKWrapper to explicit kwargs so future alibi-detect signature changes fail loudly instead of silently misaligning. - Reductor.__init__(): isinstance(transforms, Compose) raises TypeError when torchvision isn't installed (Compose is None via import_optional_module); guard on Compose is not None. Also fixed `device`/`output_path` params typed as `str` with a `None` default. - Removed plot_label_distribution (unreachable dead code: indexed a DataFrame with a literal `None` variable, used icd_counts_pos before assignment on one branch, zero test coverage, no callers). - Removed ~470 lines of unused temporal-modeling scaffolding from monitor/utils.py (Data, get_data, run_model, get_serving_data, scale, daterange, get_obj_from_str, load_model/save_model, print_metrics_binary, load_ckp, get_device, get_temporal_model, Loader, and a stray __main__ demo block) - none were exported, imported elsewhere in the repo, or tested. Verified: 23 passed, 1 pre-existing skip in tests/cyclops/monitor. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- ModelCardReport.export(): current_report_metrics[0] and latest_report_metric_cards[0] were indexed unconditionally, raising IndexError whenever a report had no PerformanceMetric logged (e.g. a card with only owner/dataset/considerations info). - _process_metric_name(): raised UnboundLocalError for any metric `type` not prefixed with "Binary"/"Multiclass"/"Multilabel" (e.g. a custom metric name) since `name` was only assigned inside the prefix-matching branches. - regex_search()/regex_replace(): once the IndexError above is fixed, the Overview template still unconditionally indexes comp.metric_cards.metrics[0], which Jinja resolves to Undefined for an empty list; make both filters tolerate non-string input instead of raising TypeError deep in template rendering. Added regression tests for all three. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
exchange_extension("myfile", "csv") returned ".csv" instead of
"myfile.csv": os.path.splitext returns "" for old_ext on an
extensionless path, and file_path[:-len(old_ext)] evaluates to
file_path[:-0] == file_path[:0] == "" (Python treats -0 as 0).
Also fix test_index_axis, which asserted indices[0] twice instead of
checking indices[1] - a regression in axis-1 handling would have gone
uncaught.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…render - export()'s default output_filename was the static "model_card.html" (and .json), so every export() call into the same output_dir silently overwrote the previous report - contradicting the docstring's claim that "the file will be named with the current date and time", and defeating the trend/history comparison export() itself relies on (glob.glob for the most recent prior *.json). Default filename is now timestamped per call. - macros.jinja rendered Citation.content (raw BibTeX text, not HTML) with the `|safe` filter, bypassing autoescaping for no reason - BibTeX fields are free text that could contain unescaped markup. Graphic.image keeps `|safe` since it's documented to hold base64/HTML image content by design. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- README linked a badge to .github/workflows/integration_tests.yml, which doesn't exist (integration tests need a live synthea/cycquery database not available in CI, so that workflow was apparently removed without cleaning up the link). Replaced with the unit_tests badge, which does exist and wasn't represented in the README at all. - dependabot.yml only tracked github-actions; added a "uv" ecosystem entry so pyproject.toml/uv.lock dependencies get automated update PRs too. - Added a CodeQL workflow (python) - the repo had no static security scanning beyond pip-audit's known-vulnerability checks. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
CONTRIBUTING.md was 26 lines with no environment setup, test-running, or repo-layout guidance - previously a new contributor had to reverse engineer the uv workflow and pytest markers from pyproject.toml. Added uv sync/pre-commit install steps, how to run unit vs integration tests, and a one-paragraph-per-module repo layout section. Added a Keep a Changelog-style CHANGELOG.md, seeded with the fixes made so far on this branch, to be finalized under a 0.3.0 heading at release time. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- cyclops.monitor.rst's autosummary only listed clinical_applicator and synthetic_applicator, omitting Detector, Reductor, TSTester, DCTester, and Explainer (the actual public API surface) from the generated API reference. - tutorials_monitor.rst (the drift-detection tutorial page) wasn't included in any toctree, making it unreachable from the docs site navigation despite existing and linking a real notebook. - monitoring.rst is titled "Monitoring" but only covers report-card performance-over-time tracking; it never mentioned cyclops.monitor's statistical drift detection at all, so the two "monitoring" concepts in the repo were undiscoverable from each other. Added a cross-link. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Add Detector.detect_shift_by_subgroup(), which runs the already-fit tester independently on each subgroup of a target dataset defined by a SliceSpec (e.g. age band, sex, hospital site), instead of only testing the aggregate population. Motivation: a model can look stable when tested against the whole target population while drifting badly for a specific clinically or socially relevant subgroup - an aggregate two-sample test can mask this entirely (Simpson's-paradox-like effect), which matters a lot for health-equity-aware monitoring of deployed clinical models. This reuses the existing SliceSpec machinery already used by ClinicalShiftApplicator and cyclops.evaluate, so it composes with any existing slicing config. Includes Bonferroni correction across subgroups (opt-out via correction="none") to control the false-positive rate from testing many subgroups at once, and a min_sample_size guard that skips (rather than unreliably tests) underpowered subgroups while still reporting their size. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Add DCTester.explain_shift(), which explains a detected shift using SHAP on the domain classifier trained internally by tester_method="classifier" (Lopez-Paz & Oquab, 2017): that test already trains a classifier to discriminate reference vs. target samples, so SHAP on its predict_proba directly answers "which features make a sample look like it's from the shifted distribution" - the features most responsible for the detected drift. Returns a dict of feature name -> mean absolute SHAP value, sorted by descending importance. Explainer previously existed but was never wired into anything else in cyclops.monitor. Also fixed a small pre-existing bug in Explainer where background `data` was silently dropped for the default/generic shap.Explainer path (only the tree/deep/gradient branches used it). DCTester.fit() now stores the fitted source data (X_s) so explain_shift can use it as SHAP background data without requiring it to be passed again. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
cyclops.evaluate.metrics.experimental had no calibration metrics despite this being one of the biggest gaps for clinical ML evaluation: discrimination metrics like AUROC say nothing about whether a predicted probability can be trusted at face value, which matters when risk scores are acted on directly (e.g. a 30% predicted mortality risk should correspond to an observed 30% event rate). Adds, following the existing array-API-agnostic (numpy/torch/cupy) functional+class metric pattern: - BinaryBrierScore / MulticlassBrierScore + binary_brier_score / multiclass_brier_score: mean squared error between predicted probabilities and the (one-hot) target, a proper scoring rule. - BinaryCalibrationError + binary_calibration_error: bins predicted probabilities and measures the gap between average confidence and observed accuracy per bin. The default "l1" norm is the standard Expected Calibration Error (ECE); "max" gives MCE. Both support logits (auto-sigmoid), ignore_index, and streaming update()/compute() accumulation, verified to match sklearn's brier_score_loss and hand-computed reference values, across numpy and torch backends. Multiclass calibration error is intentionally out of scope for this change (top-label vs. per-class averaging is a real design decision, not a mechanical extension of the binary case). Full experimental metrics regression suite (9108 tests) still passes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…rness() Both evaluate() (the top-level public API for evaluating models on a dataset) and evaluate_fairness() (989 lines, the module's fairness/ subgroup evaluation entry point) had zero test coverage - the only exercise they got was indirect, through cyclops.tasks.classification. Covers: basic overall evaluation, per-slice results via SliceSpec, multiple prediction columns, empty-slice behavior (both raise and warn-with-NaN paths), missing-column validation, DatasetDict split handling, evaluate()'s fairness_config integration, categorical and continuous (group_bins) fairness grouping, group_base_values parity, and invalid-argument error paths. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
MLPModel (and the packaged "mlp_pt" model) was completely unusable:
- get_module("activation", activation) returns the activation *class*
(e.g. torch.nn.ReLU), not an instance; inserting it directly into
nn.Sequential raised "list is not a Module subclass" one level up
because of the next bug, but would have failed on its own regardless
since nn.Sequential requires Module instances. Now instantiates the
class, and passes an already-instantiated nn.Module through unchanged
(matching the documented `Union[str, nn.Module]` type).
- `layers = [self._layer(...)]` wrapped the first hidden layer's
[Linear, activation] list in another list instead of using it
directly, so nn.Sequential(*layers) received a list as one of its
"modules".
- The loop connecting hidden-to-hidden layers used `input_dim` instead
of `hidden_dims[i]` for the first iteration, silently building a
Linear layer with the wrong input shape whenever hidden_dims[0] !=
input_dim (i.e. essentially always, given the default hidden_dims).
- configs/mlp_pt.yaml set `model__layer_dim: 2`, a copy-paste leftover
from the RNN/GRU/LSTM configs; MLPModel.__init__ has no such
parameter, so `create_model("mlp_pt", ...).initialize()` raised
TypeError unconditionally.
Added tests/cyclops/models/neural_nets/test_mlp.py (a previously
untested module) covering construction, multi-layer shape chaining,
nn.Module-instance activations, and the packaged config.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
filter_datetime()'s `day` argument called pc.year(example_values) instead of pc.day(example_values), so slicing on day-of-month (e.g. SliceSpec's datetime component slices) silently filtered on year instead - a wrong-results bug, not a crash, in a function used throughout evaluate/monitor/report slicing. The existing parametrized test for this (test_filter_datetime) requires a live Synthea database and is excluded from CI via @pytest.mark.integration_test, so this went uncaught. Added a self-contained unit test (no database) that exercises filter_datetime directly against a synthetic pyarrow Table. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Full test suite (9938 tests, excluding integration tests requiring a live database) passes cleanly on this branch. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
CI on PR #771 failed: "CodeQL analyses from advanced configurations cannot be processed when the default setup is enabled". The repo already has GitHub's default CodeQL setup configured (covering python, javascript-typescript, and actions) via repository security settings, which isn't visible as a workflow file - my earlier repo audit only checked .github/workflows/, so I missed it and added a redundant "advanced configuration" workflow that GitHub refuses to run alongside the existing default one. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
CI's doctest hook (python -m doctest) failed two ways after the explain_shift() change: 1. cyclops/monitor/detector.py's detect_shift_by_subgroup() docstring example referenced an undefined `detector` variable - replaced with a genuinely runnable example that constructs a real Detector, fits it, and calls the method. 2. shap depends on a third-party package also named `slicer`. Python's `python -m doctest file1.py file2.py ...` inserts each file's own directory onto sys.path and imports it by bare filename before restoring sys.path - so doctesting cyclops/data/slicer.py makes it importable as bare `slicer`, and that import gets cached in sys.modules. If shap is imported anywhere later in the same doctest invocation, it picks up our cached `slicer` module instead of the real pip-installed one and fails with "ImportError: cannot import name 'Alias' from 'slicer'". This was latent until this PR touched both cyclops/data/slicer.py and (via explain_shift) something that imports shap in the same commit. Renaming cyclops/data/slicer.py is out of scope (SliceSpec is public API used throughout the codebase). Instead: made cyclops/monitor/explainer.py's shap import lazy (deferred to Explainer.__init__ instead of module load) so merely importing cyclops.monitor doesn't trigger it, and skipped the one doctest line that legitimately calls explain_shift() (and therefore always imports shap) from execution. Verified locally by running `python -m doctest` across every file changed in this PR (in original and reversed order) and across the entire cyclops/ tree, matching the CI hook exactly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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
1 participant
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
A broad pass over the codebase focused on the
cyclops.monitor(drift detection) module, since it's the most novel/differentiated part of this toolkit, plus targeted fixes and new features acrossevaluate,report,models,data, and repo infrastructure. 16 commits, each independently reviewed, tested, and pushed.New features
Detector.detect_shift_by_subgroup()): runs drift tests independently per subgroup (age band, sex, hospital site, ...) instead of only on the aggregate population — a model can look stable overall while drifting badly for one group, which matters for health-equity-aware monitoring. Includes Bonferroni correction and a minimum-sample-size guard.DCTester.explain_shift()): SHAP-based explanation of why a domain-classifier-based drift test fired, ranking features by how strongly they indicate a sample belongs to the shifted distribution.BinaryBrierScore/MulticlassBrierScore,BinaryCalibrationError): closes a real gap for clinical risk models — discrimination metrics like AUROC say nothing about whether a predicted probability can be trusted at face value.Bug fixes (all with regression tests)
cyclops.monitor: matplotlib color-cycling crash, a Bonferroni-correction state-mutation bug that compounded across repeated calls, a silently-misaligned alibi-detect wrapper (missingpreprocess_at_initshifted every later positional argument by one slot), aReductorcrash without torchvision installed.cyclops.report: twoexport()crash paths (no metrics logged; non-torchmetrics-style metric names), silent report-overwriting on repeatedexport()calls, an unnecessary unescaped-HTML render path for citation text.cyclops.models:MLPModel(and the packaged"mlp_pt"config) couldn't be constructed at all — three separate bugs (wrong activation instantiation, a double-wrapped layer list, wrong hidden-layer input dimension).cyclops.data:SliceSpec's day-of-month datetime filter silently matched on year instead (a wrong-results bug, not a crash, in code used throughout evaluate/monitor/report slicing).cyclops.utils:exchange_extension()dropped the filename for extensionless paths.Test coverage
evaluate()andevaluate_fairness()(989 lines) had zero test coverage despite being the module's main public entry points — added integration tests covering slicing, fairness grouping, empty-slice handling, and error paths.MLPModel/mlp_ptfixes and theSliceSpecday-filter fix (self-contained, no database required, unlike the existing integration-marked datetime tests).Cleanup
cyclops.monitor.utilsthat was neither exported, imported elsewhere, nor tested.Infra / docs
uvDependabot ecosystem entry (previously onlygithub-actionswas covered).CONTRIBUTING.md(environment setup, running tests, repo layout) and addedCHANGELOG.md.0.3.0.Test plan
pytest -m "not integration_test") passes cleanly on this branch.🤖 Generated with Claude Code