Repository navigation
perf(ABCExceptions): simplify geometry and index-set proofs - #524
Conversation
|
Advisory cross-file review against Lean Pool's supervisory rubric. This is not by itself a significance, source-verification, or merge verdict. [Low risk] Simplifies proof tactics in mathematical theorems. No new merge-blocking issue was identified in the latest update. SummaryThe PR replaces broad normalization tactics with targeted proof steps in two ABCExceptions modules. The changes since the previous review only bring in upstream-generated metadata already present at the current base.
Reviews (19) · Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." |
Proof profile (new / modified Lean files)Statement changes✅ No statements changed — all 220 tracked declarations across 2 modified files kept their type; only proofs / definition bodies changed. Statements are compared textually against the merge base (comments and whitespace ignored; Compile cost of modified files — base → headHeartbeats: 2,068 → 2,068 (+0, +0.0%). Declarations: 151 → 151. Per-file wall sum: 39.0 s → 41.2 s (+5.4%). Wall clock is measured under parallel load and is noisy; heartbeats are deterministic and are the regression signal. Files: 2 compared — 0 cheaper, 0 costlier, 2 within noise (threshold: |Δ heartbeats| ≥ 1,000 and ≥ 1%). Both sides are elaborated against the merge-base build of the pool — sound when only proofs and definition bodies change; a file whose statements changed may fail there and is flagged ⚠. Heartbeats come from Mathlib's
Advisory only — never blocks merge. Full log uploaded as the |
🤖 LLM review — refactor (
|
| Aspect | Value |
|---|---|
| Scope | targeted tactic rewrite |
| Introduces tech debt | ✅ no |
| Maintainability | ✅ improved |
| Brittleness | ✅ more_robust |
| Risk | ✅ low |
The refactor makes algebraic steps and proof dependencies more explicit without losing mathematical structure or introducing actionable maintenance debt.
Tokens: 530,987 in / 2,668 out · Tier: codex-subscription · Effort: xhigh · Billing: Codex subscription quota (no API credits) · Estimated cost: $10.8198 (Standard API equivalent; uncached input) (long-context rate)
Automated review against .github/REFACTOR_REVIEW_RULES.md. Disagree? Reply on the PR; rules can be updated in a PR of their own.
|
Re-reviewed The complete two-file refactor preserves all seven geometric cases, their witnesses and statement signatures. Targeted algebra retains the proof structure; no actionable source defect remains. Another worker pushed a tree identical to the locally validated integration immediately before this cycle's push check. The push was stopped and that exact remote head was re-reviewed; no duplicate commit was created. Targeted warning-free project build, actual The older automation hold is now contained: the existing |
Vilin97
left a comment
There was a problem hiding this comment.
Reviewed head: a4b50a4698ff390709cbb584af7f9685ff1e8153
Verdict: approve. Posted as a comment review because GitHub does not permit the authenticated PR author to submit an approving review.
The ABCExceptions refactor replaces repeated ring normalization with distributivity and associative/commutative equality, preserving all seven subsum cases, their finite-set witnesses, and the intermediate bounds. The s21 argument now separates ordinary addition from the maximum's argument, and the remaining simplification dependencies are explicit. Full diff and affected-source review found no actionable regression. Exact-head CI passed the full build, index check, linters, style, and repository quality checks; these were inspected rather than rerun locally. The advisory profile reports 2,068 to 2,068 heartbeat units. Its 39.0 to 41.2 second parallel wall sum is noisy and establishes neither a material regression nor a project-wide speedup.
| Assessment | Result |
|---|---|
| Scope | targeted proof optimization |
| Introduces technical debt | No |
| Maintainability | Improved |
| Brittleness | More robust |
| Risk | Low |
The local algebra is easier to diagnose and retains the proof's useful structure, with no source change required before merging.
Findings: none.
Validation evidence: full build and gates, proof profile.
Daily disposition: needs maintainer — protected merge operationReviewed exact head
The remaining authority boundary is operational: this repository has |
Vilin97
left a comment
There was a problem hiding this comment.
Reviewed exact head c0eb46db296ad69bb034d6d46b124aa5b2a11e27 under the refactor rules. The full two-file proof rewrite preserves the seven ABC-exception estimates and the recorded declaration types; this is not a proof of abc. The recovered structural declaration audit and alternating timing measurements remain applicable because the only new integration diff is main’s README/NOTICE refresh. The rewrite replaces repeated normalization/search with explicit algebraic steps. Current required build, separation and documentation checks are green, and the full build/lint/quality job passed. The configured refactor review approves the identical proof files; all review threads are resolved. No substantive blocker remains. Disposition: normal protected squash merge. This comment substitutes for GitHub’s unavailable formal self-approval.
ABCExceptions repeatedly normalizes large arithmetic expressions merely to rearrange sums. This replaces that work with distributivity, associative/commutative equality, and small targeted ring identities, preserving every existing elaborated declaration.
Selected project: ABCExceptions (
abc-exceptions). Base:1b904d772c79dee8eb9b9611308dba2262fd45b5. Head:ebd629c8e300cf75e0252542a7936bb7823b9269. Leanv4.34.0; Mathlib5ed2965256430c3649e86755f9576b54eca72435(unchanged).Changes and selection evidence
GeometryBound.subSums: replace fourteenring_nfcalls with explicitmul_addrewriting andac_rfl; preserve all seven mathematical cases and their chosen finite sets.GeometryBound.s21_application_basic: normalize finite sums with an explicit simp list, then prove the additive and maximum arguments separately instead of normalizing two hypotheses and the entire conjunction.GeometryBound.left_comm/right_comm: prove the required permutations by associativity/commutativity.FourierBound.symm: restrict simplification tomax_comm.mem_indexSet: replaceaesopwith explicit cast and conjunction simplification.Among inspected candidates, ABCExceptions offered two ~1,600-line proof modules initially building in 7.2–7.4 s, compared with Apportionment's 2.3 s main module; ChannelCapacity's inspected imports were already focused. Profiling identified the repeated normalization above. This is a focused proof optimization, with two files changed and no statement, definition, import, notation, namespace, metadata, or gate changes.
Semantic invariant
645/645 pre-existing declarations match exactly; none removed or changed. The head has 653 declarations: eight additional compiler-generated simplifier lemmas, with no reused names.
Before editing, a standalone Lean environment inspector recorded all project declarations, including private and generated declarations. It imports at
.privatelevel and records structuralExprrepresentations of fully elaborated types, universe parameter lists, names, declaration kind, safety/partial flags, exported visibility, reducibility/inductive/constructor/recursor metadata, and all non-theorem values. Only theorem proof bodies are excluded. After rebuilding, the comparison requires equality for every field of every base record and reports all additions separately. All eight additions are_simp_...helpers supporting the optimized proofs. The source statement comparison also reports no changed, renamed, added, or removed source declarations.Manifest SHA-256:
780598d516394c1e5a56600f969b7904fb83e674105c9f03148d41166cd4e7e3b468735c2be2d4593221bb1adb7337cedff4e928b0a31249a1cceda8bdf490e2Measurements
Same Azure VM, dependency cache and pinned toolchain. Each project trial deletes only ABCExceptions' build outputs under
.lake/build/lib/lean/LeanPool/and.lake/build/ir/LeanPool/, then runslake build LeanPool.ABCExceptions. Dependencies remain warm; these are project-clean builds, not cold dependency builds.The isolated trials alternate base/head order, re-elaborate the original and optimized source copies with
lake env lean -j1 --setup .lake/build/ir/LeanPool/ABCExceptions/Section4.setup.json <source>, and include process/import overhead. The setup supplies the unchanged repository options and module identity. Imports of Section4 are unchanged. No improvement in the default parallel project wall time is claimed. Its slower Section2 module remains on the critical path.Observed raw profiler heartbeat counts (same profiler flags on both versions; these include profiling overhead and are not wall times or the scaled
maxHeartbeatsunits):GeometryBound.subSumsGeometryBound.s21_application_basicFourierBound.symmGeometryBound.left_commGeometryBound.right_commmem_indexSetSection4's
.olean.privateshrank from 6,231,032 to 5,912,664 bytes (5.1%). Imported environment closure remains 3,549 modules; no dependency-closure reduction is claimed.Validation
lake build LeanPool.ABCExceptions: three clean trials per revision, all passed; optimized build logs contain no warnings/errors.lake exe runLinter LeanPool.ABCExceptions: passed, including all imported project declarations and slow linters.lake exe lint-style LeanPool.ABCExceptions: passed; an additional scoped root checks all seven project source files.lake exe mk_all --check: passed, no generated index changes required.python3 -m lean_pool.quality --repo <project-scope>: passed all existing checks, including source gates, size limits, project/card validation, axiom and environment-backdoor audits. The scope contains byte-identical copies of all seven project source files, its unmodified registry entry and repository configuration, a generated project-only root, and the same compiled project artifacts. No checker or gate was modified. The full pool was not rebuilt/audited locally.Lean.collectAxiomsaudit over all 653 declarations: onlyClassical.choice,propext, andQuot.soundpermitted; passed.git diff --check: passed.lake shake --force --keep-public --explain LeanPool.ABCExceptions: inspected. The existingmodulestructure is retained. Shake suggests only two pre-existing redundant public tactic imports around the custom positivity extension; these are retained for public/metaprogram stability.--forcebypasses shake's whole-workspace cache prerequisite, after the targeted project build; it does not bypass compilation or a repository gate.origin/mainwas fetched and verified equal to the recorded base immediately before commit. Work is isolated from/data/lean-pool; the original checkout was left unchanged.Profiler command for each changed file:
lake env lean -Dtrace.profiler=true -Dtrace.profiler.useHeartbeats=true \ -Dtrace.profiler.threshold=100000 LeanPool/ABCExceptions/Section4.lean # Repeat for Section2.lean at each revision.The older count-heartbeats linter under-reported module proof work in exploration; those results are excluded. Remaining limitations are the unchanged parallel build wall time, unchanged imports, profiler overhead, and lack of a local full-pool build. CI remains required; this PR is not merged automatically.
Reproducible declaration-manifest inspector
Save this outside tracked content as
Manifest.lean, then runlake env lean --run Manifest.lean base.jsonlon the built base and... head.jsonlon the built head. Match rows byname; every base row must be identical in head. Additional rows must be inspected separately.