Review Py4vasp

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.