-
Notifications
You must be signed in to change notification settings - Fork 1.1k
feat(Geometry/Manifold): the interior of a manifold is open #33189
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
Open
peabrainiac
wants to merge
12
commits into
leanprover-community:master
Choose a base branch
from
peabrainiac:ModelWithCorners-interior_isOpen
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
f745a1d
main lemma
peabrainiac 597e517
more lemmas and docstrings
peabrainiac 8b3a10f
move lemmas, make imports private
peabrainiac 25e604c
merge master
peabrainiac f627987
apply some smaller suggestions
peabrainiac b2d5c9c
factor out lemma
peabrainiac d4cd19b
golf and simplify proof some more
peabrainiac 04f19a8
apply some more suggestions
peabrainiac e600bf1
clean up second long rewrite too
peabrainiac b8c62e5
extract a few things as variables
peabrainiac e8fe461
improve docstrings
peabrainiac 38721e6
apply suggestions
peabrainiac File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be called
extendCoordChange.I also think we should use it in
contDiffOn_extend_coord_change(and perhaps rename that one asContDiffOn.extendCoordChange)... I'm not sure if this will have further fall-out in mathlib; maybe that alone should be a previous PR.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure it should? It looks like both
extend_coord_changeandext_coord_changecurrently appear in mathlib lemmas, so I imagined either name would be fine - andextCoordChangefitsextChartAt, so I somewhat prefer it.About using the API in existing lemmas like
contDiffOn_extend_coord_change, would you be okay with leaving that to a follow-up PR? I think you're right that it would've been nice to do that in an earlier PR already, but seeing as we already have two PRs depending on this one now, it would be nice to get this unblocked instead of opening yet another prerequisite PR.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect
extCoordChangeto be aboutextChartAt, whereas this definition is aboutextending any chart - so yes, I am sure :-) (Actually checking with the existing names confirms this.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My hope is that the other PR can go in quickly, and then won't block this for long. Can we try that, and if it takes longer than a few days, I'll be happy to merge this PR as-is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I look forward for orientations to be merged into mathlib, but I'm not sure if this is as critical. From my perspective, other work depending on this PR... well, happens. As long as everything is moving reasonably fast, I have learned to live with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see - I hadn't realised that the difference between
ext_coord_changeandextend_coord_changelemmas is whether they were are aboutextChartAtor general extended charts, I thought it was just inconsistent.I've opened a separate PR for this now.