Add evaluation harness and hardness knob for detector benchmarking - #16
opensource-SantanderAI merged 4 commits into
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
recheck |
opensource-SantanderAI
left a comment
There was a problem hiding this comment.
Thanks @fabio-rovai — this is genuinely valuable: the repo advertises detector benchmarking but ships no scoring and trivially-separable defaults, and the evaluation harness + hardness knob close exactly that gap. The 12 tests in test_evaluate.py are real (32 assertions) and all pass. Two CI gates are red and need fixing before we merge:
1. Coverage gate (Test (Python 3.10/3.11/3.12) fail). Required coverage is 90%; this PR lands at 88.6%. The new src/gen_fraud_graph/evaluate.py is only 56% covered (62/140 lines) — the untested parts are mainly the CLI/main entrypoint and the result/output formatting (roughly lines 85-101, 119-129, 275-318, 323-343, 348-352). Adding tests that exercise the CLI path and the printed/structured output should clear the 90% gate.
2. Lint gate (Lint & format & type-check fail on Black --check). Black reports the code is formatted for 3.12 and fails to verify under 3.11. Please run Black/ruff with the project's supported target (--target-version py310/py311) and push, e.g. ruff check --fix . && black . from a 3.10/3.11 environment (or set target-version in pyproject.toml).
No hidden issues beyond these — once coverage is ≥90% and lint is green, we'll be glad to merge. Thanks!
Addresses the two red gates from review on PR SantanderAI#16: - Lint: Black --check failed because evaluate.py's multi-line print statements were not in canonical form. Reformatted with the project's pinned Black target (py310-py312); ruff and `mypy src/` stay green. - Coverage: total was 88.6% (<90% gate); evaluate.py was 56% covered. Added 8 tests exercising the previously untested paths: the edge/CSV and headerless flag loaders, discover_account_universe, the empty-ring branch, the missing-fraud_cases FileNotFoundError, and the CLI (run_cli human + --json, main entrypoint, _print_summary). evaluate.py is now 99% covered and total coverage is 98.4%; the full suite is 62 tests passing. Defaults and behaviour unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks @fabio-rovai — this is in great shape now. The earlier review points are fully addressed: coverage is at 98% and Black/lint is green, and the full CI suite passes (Lint, Test 3.10/3.11/3.12, CodeQL, license allowlist, pip-audit, SPDX, pattern scan). 🎉 There's just one thing left before we can merge:
Since these are logic files that both your branch and
Once it's conflict-free and CI is green again, ping us and we'll re-review and merge right away. Thanks again for the solid work on the evaluation harness + hardness knob! |
The README frames this generator as a way to benchmark fraud detectors, but it
ships no scoring and the default data is easy to separate by trivial heuristics
(single sentinel fraud amount, disjoint rings, no legitimate cycles).
This change is additive and backward compatible (low hardness reproduces the
original behaviour exactly):
- evaluate.py + gen-fraud-graph-evaluate CLI: precision/recall/F1 at account and
ring level against fraud_cases.csv, with a confusion summary.
- --hardness {low,medium,high}: jitters fraud amounts, overlaps rings, and
injects decoy legitimate high-value cycles so amount-thresholding and
cycle-topology each fail.
- tests/test_evaluate.py: 12 tests (exact metrics on a fixture, ring threshold,
loaders, hardness presets, generator smoke test). Full suite 54 passed.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Addresses the two red gates from review on PR SantanderAI#16: - Lint: Black --check failed because evaluate.py's multi-line print statements were not in canonical form. Reformatted with the project's pinned Black target (py310-py312); ruff and `mypy src/` stay green. - Coverage: total was 88.6% (<90% gate); evaluate.py was 56% covered. Added 8 tests exercising the previously untested paths: the edge/CSV and headerless flag loaders, discover_account_universe, the empty-ring branch, the missing-fraud_cases FileNotFoundError, and the CLI (run_cli human + --json, main entrypoint, _print_summary). evaluate.py is now 99% covered and total coverage is 98.4%; the full suite is 62 tests passing. Defaults and behaviour unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
206163d to
6b79e20
Compare
|
Rebased onto latest The conflict was semantic rather than textual: #14 made each ring's accounts disjoint, which collided with this PR's Local verification (mirrors CI): |
|
recheck |
62a1db9 to
c5bc900
Compare
|
Updated: merged latest |
opensource-SantanderAI
left a comment
There was a problem hiding this comment.
All previously requested changes are fully addressed: coverage at 98.3%, Black/ruff/mypy clean, the semantic conflict with #14 (disjoint rings vs. ring_overlap) resolved by making overlap opt-in, and the full CI suite is green on the updated branch (Lint, Test 3.10/3.11/3.12, CodeQL, license allowlist, pip-audit, SPDX, pattern scan). This closes the repo's core gap — detector scoring (P/R/F1) plus a hardness knob for non-trivial data. Approving for merge. Thanks @fabio-rovai for the thorough iteration!
Add an evaluation harness and a hardness knob
Why
The README describes this project as a way to "benchmark graph-based fraud
detection models". Today the generator produces data but ships no way to score a
detector against the ground truth, and the default data is easy to separate by
trivial heuristics: every fraud edge carries the exact same sentinel amount
(9999.00), rings are disjoint, and there are no legitimate cycles. A naive
"flag every account on a high-value cycle" baseline scores near-perfectly on the
defaults, so the generator cannot currently tell a good detector from a bad one.
This PR closes both gaps, additively and backward compatibly (defaults
unchanged).
What
1. Evaluation harness (
evaluate.py, new).Given a generated dataset directory and a set of detector-flagged account ids (a
plain list or a CSV), it computes precision, recall and F1 at two granularities:
ring_thresholdof itsaccounts are flagged; default 1.0 = all).
It reads ground truth from
fraud/fraud_cases.csv, reports a confusion summary(with true negatives when an
accounts/directory is present), and exposes a CLIvia a new
gen-fraud-graph-evaluateconsole script (--data,--flagged,--ring-threshold,--json).2. Hardness knob (
--hardness {low,medium,high}).A difficulty preset, wired through
Config, that makes the data harder toseparate by trivial heuristics:
amount_jitter: spread fraud amounts around the sentinel so the amount aloneis no longer a giveaway,
ring_overlap: let rings share accounts (overlapping rings, not just disjointones),
decoy_ratio: inject legitimate high-value cycles into the normal transactionstream (
transactions/transactions_decoy.csv, never written tofraud_cases.csv), so pure amount-thresholding and pure cycle-topology eachfail.
low(the default) reproduces the original behaviour exactly, so existing userssee no change.
Tests
tests/test_evaluate.pyadds 12 tests: exact precision/recall/F1 on a hand-builtfixture, ring-threshold behaviour, the loaders, an end-to-end
evaluate_dataset,the hardness presets, and a generator smoke test asserting that
highhardnesswrites decoy cycles and jitters fraud amounts while
lowstays backwardcompatible. The full suite (54 tests) passes.
Notes
modified.
the Santander CLA before merge, and I will sign it on this PR.