Adopt the py-canon standard, and read Parquet - #17
Merged
Conversation
Three things, all of which had to happen together because the standard's linters are what surfaced the first. A bug. `estimate(max_dependence_points=N)` did nothing. The argument is documented, `estimate_from_file` forwards it, and the diagnostics function reads it and subsamples above it -- but the call inside `estimate` omitted it, so every run used the 2500 default. ruff's ARG001 found it: an accepted, documented, unread parameter. The same shape as the ci_level bug in #12. Two regression tests, watched failing. CSV is no longer the interface. `estimate_from_csv` becomes `estimate_from_file`, dispatching on the suffix: .parquet/.pq, or .csv/.tsv with optional compression. `read_frames` exposes the reader on its own. Parquet is what the docs show now, because CSV carries no types: a count column with one missing value comes back float, and timestamp comes back a string something downstream has to parse. Tests assert the two formats agree to twelve places and pin the dtype difference. The standard itself. CI, docs, release and Dependabot call py-canon's reusable workflows; python-publish.yml is superseded by release.yml. ruff at the fleet configuration, pyright instead of mypy, pydoclint on docstrings. That took the lint count from 326 to zero -- mostly by pointing per-file-ignores at geoinference/tests/ rather than the tests/ the template assumes, allowing the multiplication sign the codebase uses for grid dimensions, and exempting the two modules whose printed output is their interface. The rest was real: nine missing docstrings, two missing Raises sections, and a test that unpacked a possibly-None interval. The Python floor rises to 3.12 with the fleet. The package has never been published, so nothing depended on 3.11. preen check --strict: 21 issues -> exit 0, one info-level note about the flat layout left. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YDg9BEDJgFhsixKFjAquNm
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
CI caught two things the local run could not. pyright on the runner resolves `geo_sampling`, which this machine cannot, so it type-checked attribute access that locally stopped at an unresolved import. `sample_points` calls `geo_sampling.sample_roads_for_region` and `geo_sampling.RoadSampler`. Neither exists. `geo_sampling` exports nothing at its top level -- it is two CLI scripts, `geo_roads` and `sample_roads`, whose modules expose `main` and `write_csv`. So the function raises AttributeError on the first call, and always would have. Deleted, along with the example's `--live` flag that was its only caller. Building a universe from a roads file with `points_from_roads` is what the example and the tests already do. The docs job runs doctests. The `>>>` blocks in the package docstring and in `estimate` referenced a `df` that was never defined, so they failed the moment anything executed them. They now build their own frame and assert a value: four frames, 14 women of 40 people, ratio 0.35. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YDg9BEDJgFhsixKFjAquNm
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
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.
Three things that had to happen together, because the standard's linters are what surfaced the first one.
A bug:
max_dependence_pointsdid nothingestimate(max_dependence_points=N)is documented,estimate_from_fileforwards it, and_dependence_diagnosticsreads it and subsamples above it — but the call insideestimateomitted the argument, so every run used the 2500 default. Asking for a cheaper diagnostic on a large frame set changed nothing.ruff's
ARG001found it: an accepted, documented, unread parameter. Same shape as theci_levelbug in #12. Two regression tests, both watched failing with the fix reverted:CSV is no longer the interface
estimate_from_csv→estimate_from_file, dispatching on the suffix:.parquet/.pq, or.csv/.tsvwith optional compression. A newread_framesexposes the reader on its own.Parquet is what the docs show now, and the reason is not taste: CSV carries no types, so a count column with one missing value comes back as
float64andtimestampcomes back as a string something downstream has to guess the format of. Two tests pin this — one asserts the formats give the same estimate to twelve places, the other asserts the dtype difference is real.No compatibility alias. The package has never been published (PyPI 404s), so nothing imports the old name.
The standard
CI, docs, release and Dependabot now call
gojiplus/py-canonreusable workflows instead of carrying copies;python-publish.ymlis superseded byrelease.yml. ruff at the fleet configuration, pyright instead of mypy, pydoclint on docstrings, plusCHANGELOG.md,CITATION.cff, pre-commit and zizmor.That started at 326 ruff findings. Most were the config landing wrong rather than real problems:
per-file-ignorespointed attests/**, but the tests live atgeoinference/tests/**— so nothing was exempt. Fixing the path, and addingPT009/PT027because the suite isunittest.TestCasethroughout and rewriting 140 assertions buys nothing, cleared 235.allowed-confusables = ["×"]: the codebase writes grid dimensions as100 × 100, which is what the notation means.T201exempted forio.pyandsimulate.py, whose printed output is their interface.The residue was real and is fixed: nine missing docstrings, two missing
Raisessections, a lambda-as-def pair, seven over-long lines, and a test that unpackedratio_ci.wildwithout checking it wasn'tNone.The Python floor rises to 3.12 with the fleet. Never published, so nothing depended on 3.11.
Verification
Also merged ahead of this: #12 (the
ci_levelfix, independently re-verified) and #5/#6/#7/#16. Note that #5/#6/#7 bumped actions inci.yml/docs.yml, which this PR replaces with reusable-workflow callers — so those bumps are now moot, but merging them cost nothing and kept the queue clean.🤖 Generated with Claude Code
https://claude.ai/code/session_01YDg9BEDJgFhsixKFjAquNm