Skip to content
This repository was archived by the owner on Jul 24, 2024. It is now read-only.

Conversation

sgouezel
Copy link
Collaborator

@sgouezel sgouezel commented Feb 18, 2020

There are three serious problems with the current implementation of C^n functions in mathlib:

  • Being C^n in a domain is defined by requiring that some choice of the iterated derivative (given by iterated_fderiv) is continuous up to n. In a domain where the derivative is not unique, this creates problems (for instance, the sum of two C^n functions might not be C^n). This means that the notion is well behaved only in sets with unique derivatives. As a consequence, further files using this API spend a lot of time checking the unique derivative property, while this could be avoided with a more clever choice of definition.
  • The n-th iterated derivative is defined currently as the n-1-th derivative of the derivative. Hence, it is a function in E -> (E -> (E -> .... (E -> F))...), which is pretty much unusable from a practical point of view.
  • Defining the n-th derivative as the n-1-th derivative of the derivative is a definition by induction, but generalizing the target space: to define the n-th derivative of a function from E to F, one needs to know how to define the n-1-th derivative of a function from E to E -> F. Such an inductive definition only makes sense if the spaces F and E -> F are in the same universe, i.e., if E and F are in the same universe. This is the choice that is made currently, preventing universe polymorphism.

This PR solves these three issues, essentially by rewriting the file analysis/calculus/times_cont_diff_on from scratch.

  • Being C^n on a domain is now defined as the existence of a sequence of derivatives with the right properties. Being on a domain with unique derivatives becomes irrelevant for most statements.
  • The n-th derivative is now defined as a multilinear map, by adding suitable currification steps in the inductive definition.
  • We define now the n-th derivative as the derivative of the n-1-th derivative. This avoids universe issues, at the cost of having somewhat more complicated proofs in some places (as inductive proofs are not really usable any more). In fact, it turned out to be not so bad. The only tricky place is the proof that the composition of C^n maps is C^n, where the inductive proof is really the good one. In the PR, I do the induction when all spaces are in the same universe, and then I use some lifting trick to generalize it to the general situation.

TL;DR The old file is bad. The new file is much better.

@sgouezel sgouezel added the blocked-by-other-PR This PR depends on another PR which is still in the queue. A bot manages this label via PR comment. label Feb 18, 2020
@kim-em kim-em removed the blocked-by-other-PR This PR depends on another PR which is still in the queue. A bot manages this label via PR comment. label Feb 21, 2020
@sgouezel sgouezel added the awaiting-review The author would like community review of the PR label Feb 21, 2020
@kim-em
Copy link
Collaborator

kim-em commented Feb 24, 2020

Looks fantastic (I only attempted to read the comments and statements, none of the code.)

sgouezel and others added 6 commits February 24, 2020 21:18
Co-Authored-By: Scott Morrison <scott@tqft.net>
Co-Authored-By: Scott Morrison <scott@tqft.net>
Co-Authored-By: Scott Morrison <scott@tqft.net>
Co-Authored-By: Scott Morrison <scott@tqft.net>
Co-Authored-By: Scott Morrison <scott@tqft.net>
@robertylewis
Copy link
Member

I can't review the content here, but I feel uncomfortable seeing something titled "massive refactor" sitting for over a week! @sgouezel @urkud is this ready to go, or are we waiting on comments from someone else?

@kim-em
Copy link
Collaborator

kim-em commented Feb 27, 2020

I understand the mathematical issues and this seems a good resolution of the previous problem. I never attempted to read the actual proofs, but the documentation and statements are great, and I'm happy to trust @sgouezel's judgement constructing nice enough proofs...

@kim-em
Copy link
Collaborator

kim-em commented Feb 27, 2020

I think we should merge now.

@PatrickMassot PatrickMassot added ready-to-merge All that is left is for bors to build and merge this PR. (Remember you need to say `bors r+`.) and removed awaiting-review The author would like community review of the PR labels Feb 28, 2020
@mergify mergify bot merged commit 4637e5c into leanprover-community:master Feb 28, 2020
butterthebuddha pushed a commit to butterthebuddha/mathlib that referenced this pull request May 15, 2020
…over-community#2012)

* feat(data/fin): append

* Update fin.lean

* Update fintype.lean

* replace but_last with init

* cons and append commute

* feat(*/multilinear): better multilinear

* docstrings

* snoc

* fix build

* comp_snoc and friends

* refactor(analysis/calculus/times_cont_diff): massive refactor

* fix docstring

* move notation

* fix build

* linter

* linter again

* Update src/analysis/calculus/times_cont_diff.lean

Co-Authored-By: Scott Morrison <scott@tqft.net>

* Update src/analysis/calculus/times_cont_diff.lean

Co-Authored-By: Scott Morrison <scott@tqft.net>

* Update src/analysis/calculus/times_cont_diff.lean

Co-Authored-By: Scott Morrison <scott@tqft.net>

* Update src/analysis/calculus/times_cont_diff.lean

Co-Authored-By: Scott Morrison <scott@tqft.net>

* Update src/analysis/calculus/times_cont_diff.lean

Co-Authored-By: Scott Morrison <scott@tqft.net>

* curryfication -> currying

Co-authored-by: Scott Morrison <scott@tqft.net>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
butterthebuddha pushed a commit to butterthebuddha/mathlib that referenced this pull request May 16, 2020
…over-community#2012)

* feat(data/fin): append

* Update fin.lean

* Update fintype.lean

* replace but_last with init

* cons and append commute

* feat(*/multilinear): better multilinear

* docstrings

* snoc

* fix build

* comp_snoc and friends

* refactor(analysis/calculus/times_cont_diff): massive refactor

* fix docstring

* move notation

* fix build

* linter

* linter again

* Update src/analysis/calculus/times_cont_diff.lean

Co-Authored-By: Scott Morrison <scott@tqft.net>

* Update src/analysis/calculus/times_cont_diff.lean

Co-Authored-By: Scott Morrison <scott@tqft.net>

* Update src/analysis/calculus/times_cont_diff.lean

Co-Authored-By: Scott Morrison <scott@tqft.net>

* Update src/analysis/calculus/times_cont_diff.lean

Co-Authored-By: Scott Morrison <scott@tqft.net>

* Update src/analysis/calculus/times_cont_diff.lean

Co-Authored-By: Scott Morrison <scott@tqft.net>

* curryfication -> currying

Co-authored-by: Scott Morrison <scott@tqft.net>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
@sgouezel sgouezel deleted the better_times_cont_diff branch May 26, 2020 12:18
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
ready-to-merge All that is left is for bors to build and merge this PR. (Remember you need to say `bors r+`.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants