refactor: fix the package, tooling and identification boundaries - #1
Open
r-uben wants to merge 11 commits into
Open
refactor: fix the package, tooling and identification boundaries#1r-uben wants to merge 11 commits into
r-uben wants to merge 11 commits into
Conversation
Scaffolds docs/plans/src-layout-refactor/ — six tickets across three waves, targeting three evidenced defects only. The flat 16-module layout is explicitly out of scope: at 3,054 lines it is a reasonable shape, and reorganising it for tidiness would churn 249 tests and the skill docs for nothing. Advisory panel was dispatched to three models and one answered. codex found a contradiction I had written into my own constraints — 'all 249 tests pass unchanged' against a ticket that must edit two test modules' imports — and it split the oversized A1 into relocate / drop-scripts / fix-docs. It also caught that the packaging guard was checking the source tree when a hatch artifacts setting can re-include files into the wheel regardless, and that a stale global console script could make the verification pass while proving nothing. Panel provenance is recorded in STATUS.md rather than presented as consensus: gemini needs an OAuth re-login and grok returned empty.
_convert.py and _graph.py shelled out to pandoc and resolved repo-relative paths (skill/graph, VAR_Handbook.tex) that no installed user has. They were 16% of the library package serving nobody who installs it. Both now live in tools/pyvartoolbox_docs/, a uv workspace member wired into the dev dependency group so the commands still resolve inside a checkout. Their two console scripts move with them; pyvartoolbox-replicate stays. Test imports updated to the new path, import-path-only.
ident._complete and sign._complete_basis were near-duplicates of the same operation — complete a unit vector to an orthonormal basis by QR, fixing the sign of the first column — split across two modules, and sign.py reached back into ident.py for _cholesky. Two copies of a routine underpinning proxy-SVAR and sign+IV will drift. Both now live in _linalg.py as a guarded cholesky() and a single orthonormal_completion(q1, nvar). The positive-definite error message is preserved verbatim. The behavioural asymmetry is deliberate and untouched: ident restores the identified column after completion, breaking B @ B.T == sigma; the plain rotation path in sign does not.
A1, B1 and A2 done, reviewed and verified. Board updated with per-ticket logs. Two corrections to the plan as written: the 249-test baseline was stale (the real baseline at a5710ec is 252, per-file identical), and A2's change landed inside A1's diff because the A1 dispatch prompt paraphrased the ticket and absorbed A2's scope. A2 was verified independently against the built wheel rather than marked done by assertion; both the board and its log say so.
impact_matrix(m, "sign") raised "not implemented", which was false: sign, narrative and sign+IV are implemented in sign.py, they just have no single B0inv for a function of that shape to return. The error now says so and points at sign_restricted_irf. The module split was real but unwritten, so it read as arbitrary: ident.py holds point-identified schemes and the dispatcher, sign.py holds the set-identified ones and the sampler. Both modules now say that, as does skill/references/api.md. _PLANNED splits into _SET_IDENTIFIED (implemented elsewhere) and genuinely-unwritten (exog alone). The sigma == B0inv @ B0inv.T claims are stated conditionally: iv and the sign+IV path preserve the identity when the instrument spans the full VAR sample and break it when it does not (measured 5.6e-16 vs 9.5e-02). An absolute claim there would have been a new inaccuracy in a commit whose purpose is removing one.
The skill docs told readers to run pyvartoolbox-convert-handbook and pyvartoolbox-graph-diagram. Those now ship with the pyvartoolbox-docs dev workspace package rather than the library, so a pip install cannot put them on PATH and the instructions promised something undeliverable. The instructions stay — they are still correct for maintainers regenerating docs — but now say so. GRAPH.md and handbook/INDEX.md are generated, so the wording was changed in the generators and the files regenerated through the console scripts; hand-editing either would satisfy a grep and then fail the regeneration comparison.
The pages fixture excluded INDEX.md as "hand-written scaffolding", which was false: INDEX.md is generator output from _index(). Only README is hand-written. The practical consequence was that nothing regeneration-tested INDEX.md, so a hand-edit drifting from the generator would pass CI, while GRAPH.md had exactly that protection. test_index_is_current mirrors test_diagram_is_current: rebuild the index from the page set and byte-compare. Verified to have teeth — appending a line to INDEX.md fails it, restoring passes.
Moving the doc tooling out of the package fixed one instance; nothing stopped the next dev-only module re-entering the wheel, because nobody inspects a built artifact by hand. These tests read the built wheel rather than the source tree, since a stray hatch packages/artifacts entry can re-include a file that src/ layout alone would have kept out. Tooling module names are derived from the workspace package rather than hardcoded, and both derived sets assert non-empty so the tests cannot pass vacuously. configparser lower-cases option names by default, which would mangle a case-sensitive script name and fail the entry-point comparison for a reason unrelated to packaging; optionxform is set to str. Latent today, since every script is lower-case. They skip when no matching wheel exists, so the suite stays runnable without uv build. That also means they never execute in CI, which never builds — tracked as TICKET-C3.
C1 and C2 done. Board, waves and next action reconciled against what actually shipped. Records three defects that passed every gate they were subject to and surfaced only by executing: B2's absolute claim about a conditional numerical invariant, C1's configparser case-folding, and C1 never running in CI at all (filed as C3). Also notes the two Done-when commands in this plan that could never pass, and that wave 2's files were disjoint while its gates were not.
tests/test_packaging.py inspects the built wheel and skips when dist/ is absent. CI ran uv sync, ruff and pytest but never uv build, so all four tests skipped on every pull request across all three Python versions while the suite reported green — the regression they exist to catch was unguarded exactly where every change is checked.
C3 required CI evidence rather than a local proxy, since the defect was that the packaging tests silently skipped in CI. Run 30197139066 reports 258 passed and 0 skipped on 3.11/3.12/3.13; the same run would previously have been 254 passed, 4 skipped. All eight tickets done. Records the two that had one verification pass rather than two, and that the advisory panel was one model throughout.
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
Three boundary fixes. The flat 16-module layout is deliberately not reorganised — no files were grouped into subpackages, no functions renamed, and the public API is byte-for-byte identical to
main.1. Distribution boundary.
_convert.pyand_graph.pyshelled out topandocand resolved repo-relative paths (skill/graph,VAR_Handbook.tex) that no installed user has — 16% of the package, plus two console scripts squatting on every user's PATH. Both moved to atools/pyvartoolbox_docs/uv workspace member, wired into the dev dependency group so maintainers keep the commands. Installed users now get onlypyvartoolbox-replicate. Skill docs updated to say these are maintainer tooling requiring a checkout.2. Duplicated numerics.
ident._completeandsign._complete_basiswere near-duplicate orthonormal-completion routines, andsign.pyreached intoident.pyfor_cholesky. Both now live in_linalg.py. The deliberate asymmetry is preserved:identrestores the identified column after completion, the plain non-IV rotation path insigndoes not.3. Two inaccuracies in the code.
impact_matrix(m, "sign")raised "not implemented" — false; sign restrictions are implemented, they just have no singleBto return. It now says that and points atsign_restricted_irf. Theident.py/sign.pysplit (point-identified vs set-identified) was real but unwritten, so it read as arbitrary; now documented in both modules andskill/references/api.md.Plus guardrails:
tests/test_packaging.pyinspects the built wheel (a hatchartifactsentry can re-include files thesrc/layout would exclude), a regeneration guard for the generatedskill/handbook/INDEX.md, and auv buildstep in CI without which the packaging tests silently skipped on every PR.Non-goals
Reorganising modules into subpackages; renaming anything public; changing numerical behaviour.
Test plan — observed results
uv run pytest -q→ 258 passed (252 onmain, +1 point-vs-set test, +4 packaging, +1 index guard)uv run ruff check .→ cleanpyvartoolbox.__all__diffed against the branch point → byte-identical_convert/_graph;entry_points.txtdeclares onlypyvartoolbox-replicatepyvartoolbox-graph-diagramresolves to the project venv, not a stale global install_convert.pyinto the package fails the packaging test; appending a line toINDEX.mdfails the index test. Restoring each returns green.GRAPH.md,INDEX.md) confirmed regenerated via the console scripts, not hand-edited — regenerating yields byte-identical outputReviewer notes
pyproject.toml— the uv workspace wiring is the genuinely novel part; the file moves are routine.pyvartoolbox-docssits in[dependency-groups] dev(PEP 735), deliberately outside project metadata so it cannot reach the published wheel's runtime deps.src/pyvartoolbox/_linalg.pyis load-bearing for proxy-SVAR and sign+IV. Theident/signasymmetry is intentional and tested — please don't "fix" it.sigma == B0inv @ B0inv.Tare stated conditionally on purpose. The identity holds when the instrument spans the full VAR sample (measured5.551e-16) and breaks when it does not (9.537e-02). An earlier draft asserted this absolutely; that was wrong.configparsercase-folding bug in the packaging test — butdocs/plans/src-layout-refactor/STATUS.mdflags them, and they merit closer reading.10in the branch name refers to nothing.Plan, per-ticket logs and the defects that only surfaced by executing rather than reading are in
docs/plans/src-layout-refactor/.