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

refactor(topology,calculus): change subset condition for composition #1549

Merged
merged 8 commits into from Oct 17, 2019

Conversation

sgouezel
Copy link
Collaborator

A grab bag of small additions that I need in further developments. I don't really think it makes sense to split it into several smaller PRs, but if you insist I will.

The main change is the reformulation of the subset condition for composition. For instance, for continuous_on, the condition for continuity of the composition of f (continuous on s) and g (continuous on t) was f '' s ⊆ t. I change it everywhere to s ⊆ f ⁻¹' t, which is equivalent but computes a lot better (belonging to the image is something complicated, with an existential quantifier, while belonging to the preimage is much simpler). This opened the way to much better simp automation for manifolds.

Copy link
Member

@PatrickMassot PatrickMassot left a comment

Choose a reason for hiding this comment

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

I made a very quick pass. The image vs preimage thing is clear: preimages are much better. But I left two docstring comments.

src/analysis/calculus/deriv.lean Outdated Show resolved Hide resolved
src/topology/local_homeomorph.lean Outdated Show resolved Hide resolved
@robertylewis robertylewis added the awaiting-review The author would like community review of the PR label Oct 15, 2019
Copy link
Member

@PatrickMassot PatrickMassot left a comment

Choose a reason for hiding this comment

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

Nice! I left a couple of stylistic suggestions, mostly hoping to help a bit making you even more efficient.

src/analysis/calculus/deriv.lean Outdated Show resolved Hide resolved
src/analysis/calculus/deriv.lean Outdated Show resolved Hide resolved
src/analysis/calculus/deriv.lean Show resolved Hide resolved
src/analysis/calculus/deriv.lean Show resolved Hide resolved
src/analysis/calculus/deriv.lean Show resolved Hide resolved
src/topology/continuous_on.lean Show resolved Hide resolved
src/topology/local_homeomorph.lean Outdated Show resolved Hide resolved
src/topology/local_homeomorph.lean Outdated Show resolved Hide resolved
src/topology/local_homeomorph.lean Outdated Show resolved Hide resolved
src/topology/local_homeomorph.lean Outdated Show resolved Hide resolved
@sgouezel
Copy link
Collaborator Author

Thanks a lot for your comments @PatrickMassot. I think I have addressed them all.

Copy link
Member

@PatrickMassot PatrickMassot left a comment

Choose a reason for hiding this comment

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

You introduced two docstring typos. Tip: if you had pushed your PR branch to the community repository instead of your fork I could have fixed the typos myself.

src/topology/local_homeomorph.lean Outdated Show resolved Hide resolved
src/topology/local_homeomorph.lean Outdated Show resolved Hide resolved
@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 Oct 17, 2019
@mergify mergify bot merged commit e5fc2a7 into leanprover-community:master Oct 17, 2019
@sgouezel sgouezel deleted the composition_subset branch December 2, 2019 10:37
anrddh pushed a commit to anrddh/mathlib that referenced this pull request May 15, 2020
…eanprover-community#1549)

* refactor(topology,calculus): change subset condition for composition

* improve docstrings

* add is_open Ioi

* reviewer's comments

* typo
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.

None yet

3 participants