Conversation
🤖 Gemini PR SummaryThis PR implements a structured and computable version of Lagrange interpolation within the Features
Refactoring
Fixes
Documentation
Analysis of Changes
Lean Declarations ✅ **Removed:** 1 declaration(s)
❌ **Added:** 18 declaration(s)
✏️ **Affected:** 1 declaration(s) (line number changed)
🎨 **Style Guide Adherence**The following lines violate the provided style guide:
📄 **Per-File Summaries**
Last updated: 2026-02-25 12:33 UTC. |
3deb5f2 to
2572be8
Compare
Co-authored-by: Julian Sutherland <julian@nethermind.io> Co-authored-by: Katerina Hristova <katherina.hristova@gmail.com>
Co-authored-by: Julian Sutherland <julian@nethermind.io> Co-authored-by: Katerina Hristova <katherina.hristova@gmail.com>
67cb474 to
9885f63
Compare
dhsorens
left a comment
There was a problem hiding this comment.
I'm very happy with this, thank you @Julek ! I just had some stylistic notes for the sake of consistency throughout the repository. But the code is correct (proven so) and I appreciate the closeness of the syntax and function definition to Mathlib's, distinctly being a Lean style. In conversation with @alexanderlhicks we agreed that the previous version of this was more Rust-y, something we may want later but can wait to develop out properly for when we have an actual use case.
I'm happy for this to be merged, though I will likely want the stylistic notes taken into account either now or later (e.g. in a code cleanup/organisation PR I'm happy to make).
Reviewed for:
- correctness
- style consistency with the rest of CompPoly
CompPoly/Univariate/Basic.lean
Outdated
|
|
||
| section Module | ||
|
|
||
| instance [BEq R] [LawfulBEq R] [Ring R] [Nontrivial R] : Module R (CPolynomial R) where |
There was a problem hiding this comment.
my personal preference for readability and reuse is to pull each field proof out into named lemmas. should note here that module theory is also implemented in #88 (still WIP) and so we can address stylistic nuances there
CompPoly/Univariate/Lagrange.lean
Outdated
| /-- Computable Lagrange basis polynomials indexed by `s : Finset ι`, defined at nodes `x i` for a | ||
| map `x : ι → F`. For `i, j ∈ s`, `basis s x i` evaluates to `0` at `x j` for `i ≠ j`. When | ||
| `x` is injective on `s`, `basis s x i` evaluates to 1 at `x i`. -/ | ||
| def basis.{u} {ι : Type u} [DecidableEq ι] (s : Finset ι) (x : ι → R) (i : ι) : |
There was a problem hiding this comment.
a note here stylistically, elsewhere in CompPoly we use {R : Type*} which is ofc compatible with the .{u} notation but we may consider keeping things consistent across the board
alexanderlhicks
left a comment
There was a problem hiding this comment.
Any chance you could do a pass over the style guide adherence (cf summary workflow)? We made a bit of an effort to clean up CompPoly in this regard and it would be nice to keep it consistent moving forward.
@alexanderlhicks Where can I find this style guide? 😅 |
|
our summary script uses the style guide from mathlib and what's outlined in CONTRIBUTING.md - if you scroll up there is a drop-down option where it gives feedback on the style guide :) |
|
one last comment here - the structure of so my personal preference would be to have ToPoly import Lagrange, and then the theorems such as wdyt @alexanderlhicks @Julek ? does that seem reasonable? I'm happy to punt this, and just make a note for future discussions |
No description provided.