Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Merged by Bors] - feat(linear_algebra/lagrange): Refactor lagrange interpolation and add extended proofs. #15036

Closed
wants to merge 26 commits into from

Conversation

linesthatinterlace
Copy link
Collaborator

@linesthatinterlace linesthatinterlace commented Jun 28, 2022

The current formulation of Lagrange interpolants phrases things in terms of a finset of nodes, and functions interpolated at those nodes. This changes things in order to allow for indexed nodes and weights. It also extends various functionalities of the lagrange interpolation.


It is more standard - and in many ways more natural - to think in terms of nodes and weights (that is, evaluations) at those nodes. This corresponds to a reformulation in terms of injective functions from fintypes. This commit implements the necessary changes to do this, and attempts to introduce API to fill the gaps left behind.

I am looking for feedback on this in terms of overall approach as well as details - I should point out that this isn't for the sake of it: in the real context of the theory of binary Goppa codes, I was encountering issues with the previous formulation as it is very natural to want to index your weights by the same thing that one indexes your nodes: and, again, it's natural for these to be tuples.

This new approach seems to give tighter and neater proofs, which seems a good sign.

@linesthatinterlace linesthatinterlace added WIP Work in progress awaiting-review The author would like community review of the PR labels Jun 28, 2022
Copy link
Collaborator

@vihdzp vihdzp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's tons of untapped simp potential.

src/linear_algebra/lagrange.lean Outdated Show resolved Hide resolved
src/linear_algebra/lagrange.lean Outdated Show resolved Hide resolved
src/linear_algebra/lagrange.lean Outdated Show resolved Hide resolved
src/linear_algebra/lagrange.lean Outdated Show resolved Hide resolved
src/linear_algebra/lagrange.lean Outdated Show resolved Hide resolved
src/linear_algebra/lagrange.lean Outdated Show resolved Hide resolved
src/linear_algebra/lagrange.lean Outdated Show resolved Hide resolved
src/linear_algebra/vandermonde.lean Outdated Show resolved Hide resolved
src/linear_algebra/vandermonde.lean Outdated Show resolved Hide resolved
src/linear_algebra/vandermonde.lean Outdated Show resolved Hide resolved
linesthatinterlace and others added 2 commits June 28, 2022 20:30
I stripped out all the simps from the original file when refactoring - this adds a chunk of them back in (though they have not been linted).

Co-authored-by: Violeta Hernández <vi.hdz.p@gmail.com>
@linesthatinterlace
Copy link
Collaborator Author

I have pushed a number of extensive reworks to this, including adding new, genuinely mathematical interesting (I think!) lemmas. https://leanprover.zulipchat.com/#narrow/stream/116395-maths/topic/Lagrange.20interpolants

@linesthatinterlace
Copy link
Collaborator Author

I have an ongoing question about a) whether the nodal stuff I define is worth keeping, b) whether it is worth defining certain special cases of the definition, and whether or not they should be accompanied by theorems.

@linesthatinterlace linesthatinterlace changed the title refactor(linear_algebra/lagrange): Refactor the lagrange interpolant in terms of fintypes. refactor(linear_algebra/lagrange): Refactor lagrange interpolation. Jul 4, 2022
@linesthatinterlace linesthatinterlace changed the title refactor(linear_algebra/lagrange): Refactor lagrange interpolation. feat(linear_algebra/lagrange): Refactor lagrange interpolation and add extended proofs. Jul 4, 2022
Copy link
Collaborator

@BoltonBailey BoltonBailey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems good. Scanning the proof no golfs jump out at me. I'll make some other reviews with style comments.

@linesthatinterlace linesthatinterlace added awaiting-review The author would like community review of the PR awaiting-CI The author would like to see what CI has to say before doing more work. and removed awaiting-author A reviewer has asked the author a question or requested changes labels Jul 22, 2022
@github-actions github-actions bot removed the awaiting-CI The author would like to see what CI has to say before doing more work. label Jul 22, 2022
@Vierkantor
Copy link
Collaborator

@Vierkantor Do you have any thoughts on whether it is worth including any of the special-case spellings a la here: https://leanprover.zulipchat.com/#narrow/stream/116395-maths/topic/Lagrange.20interpolants/near/288179709

I'd like to hold off on special spellings for now, until I see them in action a bit more. Unfortunately adding new defs generally causes a lot more work...

Copy link
Collaborator

@Vierkantor Vierkantor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went through again especially focussed on the names this time, I think one more round of reviews and we're there.

src/data/finset/basic.lean Outdated Show resolved Hide resolved
src/ring_theory/polynomial/basic.lean Outdated Show resolved Hide resolved
src/linear_algebra/lagrange.lean Show resolved Hide resolved
src/linear_algebra/lagrange.lean Show resolved Hide resolved
src/linear_algebra/lagrange.lean Outdated Show resolved Hide resolved
src/linear_algebra/lagrange.lean Outdated Show resolved Hide resolved
src/linear_algebra/lagrange.lean Outdated Show resolved Hide resolved
src/linear_algebra/lagrange.lean Outdated Show resolved Hide resolved
src/linear_algebra/lagrange.lean Show resolved Hide resolved
src/linear_algebra/lagrange.lean Outdated Show resolved Hide resolved
@Vierkantor Vierkantor added awaiting-author A reviewer has asked the author a question or requested changes and removed awaiting-review The author would like community review of the PR labels Jul 29, 2022
Copy link
Collaborator

@vihdzp vihdzp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few general remarks.

src/data/finset/basic.lean Outdated Show resolved Hide resolved
src/linear_algebra/vandermonde.lean Outdated Show resolved Hide resolved
src/ring_theory/polynomial/basic.lean Outdated Show resolved Hide resolved
@linesthatinterlace linesthatinterlace added awaiting-review The author would like community review of the PR and removed awaiting-author A reviewer has asked the author a question or requested changes labels Aug 2, 2022
@linesthatinterlace
Copy link
Collaborator Author

@Vierkantor what do you reckon now?

Copy link
Collaborator

@Vierkantor Vierkantor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks good with just some minor remarks, thanks!

bors d+

src/linear_algebra/lagrange.lean Outdated Show resolved Hide resolved
src/linear_algebra/lagrange.lean Outdated Show resolved Hide resolved
@bors
Copy link

bors bot commented Aug 3, 2022

✌️ linesthatinterlace can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

@leanprover-community-bot-assistant leanprover-community-bot-assistant added delegated The PR author may merge after reviewing final suggestions. and removed awaiting-review The author would like community review of the PR labels Aug 3, 2022
Co-authored-by: Anne Baanen <Vierkantor@users.noreply.github.com>
@linesthatinterlace
Copy link
Collaborator Author

bors r+

bors bot pushed a commit that referenced this pull request Aug 3, 2022
…d extended proofs. (#15036)

The current formulation of Lagrange interpolants phrases things in terms of a finset of nodes, and functions interpolated at those nodes. This changes things in order to allow for indexed nodes and weights. It also extends various functionalities of the lagrange interpolation.



Co-authored-by: Wrenna Robson <34025592+linesthatinterlace@users.noreply.github.com>
@bors
Copy link

bors bot commented Aug 3, 2022

Build failed (retrying...):

bors bot pushed a commit that referenced this pull request Aug 3, 2022
…d extended proofs. (#15036)

The current formulation of Lagrange interpolants phrases things in terms of a finset of nodes, and functions interpolated at those nodes. This changes things in order to allow for indexed nodes and weights. It also extends various functionalities of the lagrange interpolation.



Co-authored-by: Wrenna Robson <34025592+linesthatinterlace@users.noreply.github.com>
@bors
Copy link

bors bot commented Aug 3, 2022

This PR was included in a batch that was canceled, it will be automatically retried

bors bot pushed a commit that referenced this pull request Aug 3, 2022
…d extended proofs. (#15036)

The current formulation of Lagrange interpolants phrases things in terms of a finset of nodes, and functions interpolated at those nodes. This changes things in order to allow for indexed nodes and weights. It also extends various functionalities of the lagrange interpolation.



Co-authored-by: Wrenna Robson <34025592+linesthatinterlace@users.noreply.github.com>
@bors
Copy link

bors bot commented Aug 3, 2022

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title feat(linear_algebra/lagrange): Refactor lagrange interpolation and add extended proofs. [Merged by Bors] - feat(linear_algebra/lagrange): Refactor lagrange interpolation and add extended proofs. Aug 3, 2022
@bors bors bot closed this Aug 3, 2022
@bors bors bot deleted the lagrange-fintype branch August 3, 2022 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
delegated The PR author may merge after reviewing final suggestions.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants