Skip to content

Review checklist

Run this before a pull request is sent for review. It is built from the 313 findings of the automated reviewer on 80 pull requests (#52 to #148), grouped by kind and ordered by how often each kind came up. python tools/prereview.py runs every mechanical check. This page covers what only a reader can check.

A third of all findings were one failure mode: code changed, but a doc, a docstring, a changelog line, a number or a generated file describing it did not. Most of those findings pointed at a file the pull request never touched. So the review starts from the diff and searches outward through the whole repository, not just the changed lines.

The reviewer also reads every file a pull request touches from top to bottom, not only the changed lines: on #140 to #152, more than a third of its findings were in lines the pull request did not change, and each one cost a further review round. So the local review reads each touched file whole too.

python tools/review_ledger.py <num> adds a pull request's findings to docs/reference/review_ledger.csv and prints the tally by kind. Run it after every automated review. A kind that keeps coming up becomes a checklist item or a mechanical check.

Since #140 (the ledger)

The kinds, most frequent first; python tools/review_ledger.py prints the current counts.

kind what now catches it
input escapes validation (a raw TypeError or KeyError, an argument used before its model, a missing Integer(), a negative count) the integer-field check; the malformed-input test that arrives with #147; item 4
doc claim does not match the code (a release, a file, a count, an unmerged pull request, an uncited outside claim) the cited-path check; item 15
logic and edge cases item 3
cache and resume item 10
tooling item 9
generated file stale the class-diagram, data-dictionary and rendered-diagram checks
compatibility item 8
export missing item 8
performance item 11

What came up, and how often

kind findings pull requests
docs, docstrings or changelog out of step with the code 77 38
logic and edge cases (degenerate inputs, exhausted retries, stale state, a change only partly delivered) 44 20
input validation (integers, NaN and inf, shapes, unknown options, a raw TypeError, checking after a download) 37 14
tooling and CI (workflow inputs in shell, pagination, exit codes) 33 12
tests missing, vacuous or testing the wrong thing 23 18
cache, resume and reproducibility keys 16 7
numbers in docs that do not match the results files 15 8
performance and memory 13 10
generated files stale (diagram images, data dictionary, frozen fixture) 12 9
backward compatibility (reordered parameters, removed or renamed names) 11 11
readability contract 10 6
optional dependencies and extras 7 5
grammar and markdown 6 6
loopholes in the readability gate 5 3
type hints 4 4

The checklist

  1. Sweep for drift. For every changed name, setting, default, path or behaviour, search the rest of the repository for the old wording and fix every hit. That covers docstrings in the same module, docs/, README.md, CONTRIBUTING.md, the tool docstrings and older bullets in the same ## Unreleased section. For example: rg -n "<old name>|<old value>|<old path>" docs src tools tests README.md CHANGELOG.md CONTRIBUTING.md.
  2. Every claim holds end to end. When the change says "no longer needs X", "unchanged", "exact" or "bounded memory", trace every call path (fit, score, estimate and their helpers) and confirm it. "Unchanged numbers" means the pinned search answers and scenario tests pass unchanged.
  3. Degenerate inputs. Check 0, 1, a fraction that rounds to 0, an empty candidate list, every retry failing, a validation set with no attacked (or no benign) record, and a refit after tuning. A retry loop must never emit the last candidate it rejected.
  4. Every new input is declared on a model.
  5. An integer field carries Integer(); AtLeast alone accepts 1.5.
  6. A float that must be finite carries Finite(); NaN and inf pass a > 0 check.
  7. An option is a OneOf(<Choice>).
  8. Arrays have their shape stated.
  9. Paired lists have a length invariant.
  10. Nothing is checked in a function body, and everything is checked before any download or file read.
  11. Tests prove the claim.
  12. No or True and no assertion that holds by construction.
  13. Each new fast path and each feature branch gets its own test.
  14. A test's name matches what it loads.
  15. A test that reads a published data file runs only under FDIA_SLOW=1 (the slow workflow), never in the default suite.
  16. Numbers in docs are recomputed from the committed results file: the right metric (macro-F1 versus node-F1), the right denominator, the right rounding, and no placeholders left.
  17. Generated files are regenerated and committed together. That means the diagram .mmd, .svg and .png, and the data dictionary.
  18. Backward compatibility.
  19. A new parameter goes last or is keyword-only.
  20. A removed or renamed public name keeps a deprecated alias for one minor version.
  21. Dataclass field order does not change.
  22. New public names are exported.
  23. Tooling and CI.
  24. Workflow inputs reach a shell through env:, never through ${{ }} inside run:.
  25. List endpoints are paginated.
  26. HTTP errors other than 404 surface.
  27. A failure exits non-zero.
  28. Files and np.load are opened in context managers.
  29. Caches and resumes are keyed by every input (frames, seed, K, the source pool, the release). The release is pinned explicitly, and a corrupt cache recomputes.
  30. Memory and hot loops.
    • Nothing of size T x N is materialized in float64 on a timeline path.
    • Nothing is allocated per Newton iteration.
    • Estimate the size at IEEE-300 over 72,000 frames; python tools/bench.py --check gates the timings.
  31. The readability contract. Channel indices come from NODE/EDGE/BRANCH, file paths and attributes from schema, each formula exists once, and there are no positional tuples.
  32. Extras. A new import of scipy, torch or pandapower is declared in the matching extra, guarded with an install message, and kept off the import fdia_graph path.
  33. Prose. Read every added paragraph once for grammar, empty code fences and the docs' tone.
  34. Every factual sentence holds in this checkout.
    • A release named as the default or current matches registry._RELEASE.
    • A count (files, systems, findings) is recounted, not remembered.
    • A feature that arrives with an unmerged pull request is said to depend on it.
    • A claim about another project links the file or page that shows it.
  35. Read every touched file whole. The reviewer does, and a third of its findings sit in lines the pull request did not change. Look for the same kind of problem the change fixes (an argument still used before its model, a second copy of the old wording) elsewhere in the file.
  36. Every supported Python. CI runs 3.9 and 3.12 on Linux and one job on Windows, and the local gate runs one interpreter. A parser of loose input can differ between versions: from 3.11 date.fromisoformat also reads '20240131', which failed #147 on CI only. Pin a parsed form with a pattern before handing it to the standard library, and run the suite on a second interpreter with python tools/prereview.py --also-python <path> (or FDIA_PREREVIEW_PYTHONS, set once).

The order

  1. python tools/prereview.py until it passes.
  2. Work through the checklist above against the diff and every touched file, searching outward from every change, and fix what it finds.
  3. Only then mark the draft ready, which asks for the one automated review (the ruleset no longer reviews every push). Batch the fixes to its findings into one push, reply to each, run steps 1 and 2 again, and merge that push on green; request another review (python tools/pr.py request-review <num>) only for a fix that changes behaviour beyond what its finding asked.
  4. python tools/review_ledger.py <num> after each review, so the tally stays current.