review-py4vasp

name

review-py4vasp

description

Code review for py4vasp changes. Use when asked to review, code-review, or critique py4vasp code, a branch, or a diff. Does the usual bug / edge-case review AND enforces four py4vasp-specific rules: exceptions come from py4vasp.exception, user selections use select.Tree / index.Selector, public methods have numpy-style docstrings with runnable doctest examples, and tests give >95% coverage. Runs doctests and coverage to substantiate findings, then reports via the ReportFindings tool.

Reviewing py4vasp code

Standalone reviewer for the changes on the current branch (working tree + commits) relative to master. It combines a normal correctness review with four py4vasp house rules, and it runs the doctests and coverage rather than guessing. Evidence is gathered by .claude/skills/review-py4vasp/review.py; you read that evidence, read the diff, and file findings with ReportFindings (most-severe first). Paths below are relative to the repo/worktree root.

Step 1 — gather evidence (run the driver)

Run it with the venv interpreter (it needs pytest-cov, which the system python lacks):

.venv/Scripts/python.exe .claude/skills/review-py4vasp/review.py

Pass a different base ref as the first arg if needed (... review.py main). The driver prints five sections: changed files; coverage of each changed src module with uncovered line numbers; doctest pass/fail for those modules; a raise-convention scan; and selection-API hints. It takes ~15 s (a coverage run over the touched test files). If a changed src module has no auto-mapped test file, the driver says so — locate and run its tests manually (Step 3).

Step 2 — review against the four py4vasp rules

Read the diff alongside the driver output and judge each rule. The driver gives you leads; you decide what is a finding.

1. Exceptions come from py4vasp.exception. The contract (see the exception.py module docstring): a py4vasp exception means the user made a mistake; any other exception is a bug to report. So user-facing code must raise a Py4VaspError subclass (IncorrectUsage, NoData, DataMismatch, NotImplemented, …), not a bare ValueError/KeyError/RuntimeError. Section 4 lists every raise X in changed src where X is not exception.*. Legit exceptions (not findings): re-raising a caught error (raise err), and framework code — src/py4vasp/_sphinx/ (docutils SkipNode) and cli.py (click errors). Flag the rest.

2. User selections use select.Tree / index.Selector. If a method lets the user pick among options, the argument is named selection and is parsed with py4vasp._util.select.Tree.from_selection(...) so every quantity behaves consistently. When those selections index into an array, extraction goes through py4vasp._util.index.Selector. Section 5 shows where the changed files mention selection / select.Tree / index.Selector. Flag a new user-facing option arg that is named something else, hand-rolls its own parsing, or indexes arrays manually instead of via Selector.

3. Public methods have numpy-style docstrings with runnable examples. Every public method (no leading underscore) needs a numpy-style docstring, and user-facing quantity methods carry a >>> example that the doctest suite executes. Section 3 shows the doctest result for the changed modules — a failure or "no tests ran" for a module that gained a public method is a finding. A public method with no docstring, or a docstring with no example where siblings have one, is a finding the driver can't see — read the diff.

4. Tests cover the important behavior (>95%). Section 2 gives each changed module's coverage % and the exact uncovered lines. Below ~95% is a finding unless the only misses are trivial (e.g. an else that raises exception.NotImplemented). Point findings at the specific uncovered lines and the behavior they represent.

Also do the normal review: correctness bugs, missing edge cases, wrong results, resource/logic errors — the same scrutiny as a general code review.

Step 3 — manual commands (when the driver can't map a test)

Same invocations the driver uses, for a module it couldn't auto-map. Set PYTHONPATH to this checkout's src so a worktree measures its own edits, and use --cov=py4vasp (whole package), never a dotted submodule (see Gotchas):

PYTHONPATH="$PWD/src" .venv/Scripts/python.exe -m pytest tests/calculation/test_symmetry.py --cov=py4vasp --cov-report=term-missing -q

PYTHONPATH="$PWD/src" .venv/Scripts/python.exe -m pytest tests/test_doctest.py -k symmetry -q

Step 4 — report

Call ReportFindings with the verified findings, most-severe first (empty array if the change is clean). Give each a concrete failure scenario and a file:line. Prefer confirmed issues over speculation; the coverage/doctest sections let you confirm rather than guess.

Gotchas

• Use the venv python, not uv run, not system python. uv run pytest --cov and the venv python both crash on numpy under coverage only when you scope with a dotted module (--cov=py4vasp._calculation.symmetry): ImportError: cannot load module more than once per process (numpy 2 + Python 3.14). --cov=py4vasp (whole package) is fine — the driver always uses that and filters the report to the changed files. System python has no pytest-cov at all.
• Worktree measures the wrong src without PYTHONPATH. py4vasp is installed editable via a .pth pointing at the main repo src, so inside a .claude/worktrees/<name> worktree a bare run silently reviews the main repo. The driver forces PYTHONPATH; do the same in manual runs. Verify with python -c "import py4vasp; print(py4vasp.__file__)".
• Doctests need the project's runner. They rely on globs injected by tests/test_doctest.py (py4vasp, path, np), so pytest --doctest-modules won't work — filter tests/test_doctest.py with -k <module-stem> instead.
• The raise scan over-reports. It is a plain grep; always apply the carve-outs in rule 1 before turning a hit into a finding.

Human path

There is no app to launch — this is a review workflow. A developer reviews by reading the diff and running the same pytest --cov / doctest commands above.