Skip to content

[Merged by Bors] - refactor(Combinatorics/SimpleGraph): remove sndOfNotNil in favour of getVert 1#15237

Closed
pimotte wants to merge 20 commits intomasterfrom
PO_sndOfNotNil_refactor
Closed

[Merged by Bors] - refactor(Combinatorics/SimpleGraph): remove sndOfNotNil in favour of getVert 1#15237
pimotte wants to merge 20 commits intomasterfrom
PO_sndOfNotNil_refactor

Conversation

@pimotte
Copy link
Copy Markdown
Collaborator

@pimotte pimotte commented Jul 28, 2024

This allows to leave out the not-nil hypothesis, in addition to not needing it for
the definition of tail.


Open in Gitpod

Per discussion here

@pimotte pimotte added the t-combinatorics Combinatorics label Jul 28, 2024
@github-actions
Copy link
Copy Markdown

github-actions bot commented Jul 28, 2024

PR summary 163e433d41

Import changes for modified files

No significant changes to the import graph

Import changes for all files
Files Import difference

Declarations diff

+ adj_getVert_one
+ drop
+ getVert_cons
+ getVert_cons_one
+ getVert_cons_succ
+ getVert_copy
+ not_nil_iff_lt_length
+ support_tail_of_not_nil
+ tail_cons_eq
+ tail_cons_nil
- adj_sndOfNotNil
- cons_getVert
- cons_getVert_succ
- cons_sndOfNotNil
- getVert_one
- sndOfNotNil
- tail_support_eq_support_tail

You can run this locally as follows
## summary with just the declaration names:
./scripts/declarations_diff.sh <optional_commit>

## more verbose report:
./scripts/declarations_diff.sh long <optional_commit>

The doc-module for script/declarations_diff.sh contains some details about this script.

@ghost ghost added the blocked-by-other-PR This PR depends on another PR (this label is automatically managed by a bot) label Jul 28, 2024
@pimotte pimotte requested a review from YaelDillies July 28, 2024 19:58
@ghost ghost added merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) and removed merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) blocked-by-other-PR This PR depends on another PR (this label is automatically managed by a bot) labels Jul 29, 2024
@ghost
Copy link
Copy Markdown

ghost commented Jul 30, 2024

@ghost ghost added the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Jul 30, 2024
@pimotte pimotte force-pushed the PO_sndOfNotNil_refactor branch from 6291e86 to f1714a7 Compare July 30, 2024 15:24
@pimotte
Copy link
Copy Markdown
Collaborator Author

pimotte commented Jul 30, 2024

Just force-pushed after rebasing because of the #14977 merge:)

@ghost ghost removed the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Jul 30, 2024
@YaelDillies YaelDillies added the awaiting-author A reviewer has asked the author a question or requested changes. label Jul 30, 2024
pimotte and others added 2 commits July 30, 2024 20:12
Co-authored-by: Yaël Dillies <yael.dillies@gmail.com>
Comment on lines +775 to +779
have := adj_getVert_succ p (by
rw [SimpleGraph.Walk.nil_iff_length_eq] at hp
push_neg at hp
exact Nat.zero_lt_of_ne_zero hp)
rwa [getVert_zero] at this
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This proof really should be

Suggested change
have := adj_getVert_succ p (by
rw [SimpleGraph.Walk.nil_iff_length_eq] at hp
push_neg at hp
exact Nat.zero_lt_of_ne_zero hp)
rwa [getVert_zero] at this
simpa using adj_getVert_succ p (by simpa)

Can you add simp lemmas until something of the above works?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

simpa using adj_getVert_succ p (by simpa : 0 < p.length) works except for the second simpa, but simping "not nil" to "0 < p.length" seems pretty impactful, are you sure we want to be doing that?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I managed to get it to this:

@[simp] lemma adj_getVert_one {p : G.Walk v w} (hp : ¬ p.Nil) :
    G.Adj v (p.getVert 1) := by
  have := adj_getVert_succ p (by simpa [not_nil_iff_lt_length] using hp)
  simp only [getVert_zero, zero_add] at this
  exact this

which works

but

@[simp] lemma adj_getVert_one {p : G.Walk v w} (hp : ¬ p.Nil) :
    G.Adj v (p.getVert 1) := by
  simpa [getVert_zero, zero_add] using adj_getVert_succ p (by simpa [not_nil_iff_lt_length] using hp)

errors with

type mismatch
  h✝
has type
  G.Adj (p.getVert 0) (p.getVert (0 + 1)) : Prop
but is expected to have type
  G.Adj v (p.getVert 1) : Prop

Any clue what's going on here?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hmm... no clue, no. If you manage to minimise it, try posting it on #mathlib4 or #lean4 (depending on whether you got rid of the mathlib dependencies or not)

Copy link
Copy Markdown
Collaborator Author

@pimotte pimotte Jul 31, 2024

Choose a reason for hiding this comment

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

Type hinting the second simpa works. I don't think there's a bug with simpa, since using rwa in the second part leads to the same problem. Fixed in it that way

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Probably some metavariable instantiating too late

@pimotte pimotte removed the awaiting-author A reviewer has asked the author a question or requested changes. label Aug 2, 2024
@YaelDillies
Copy link
Copy Markdown
Contributor

Thanks!

maintainer merge

@github-actions
Copy link
Copy Markdown

github-actions bot commented Aug 2, 2024

🚀 Pull request has been placed on the maintainer queue by YaelDillies.

@github-actions github-actions bot added the maintainer-merge A reviewer has approved the changed; awaiting maintainer approval. label Aug 2, 2024
@riccardobrasca
Copy link
Copy Markdown
Member

Thanks!

bors merge

@github-actions github-actions bot added the ready-to-merge This PR has been sent to bors. label Aug 4, 2024
mathlib-bors bot pushed a commit that referenced this pull request Aug 4, 2024
…getVert 1 (#15237)

This allows to leave out the not-nil hypothesis, in addition to not needing it for
the definition of tail.
@mathlib-bors
Copy link
Copy Markdown
Contributor

mathlib-bors bot commented Aug 4, 2024

Pull request successfully merged into master.

Build succeeded:

@mathlib-bors mathlib-bors bot changed the title refactor(Combinatorics/SimpleGraph): remove sndOfNotNil in favour of getVert 1 [Merged by Bors] - refactor(Combinatorics/SimpleGraph): remove sndOfNotNil in favour of getVert 1 Aug 4, 2024
@mathlib-bors mathlib-bors bot closed this Aug 4, 2024
@mathlib-bors mathlib-bors bot deleted the PO_sndOfNotNil_refactor branch August 4, 2024 12:42
grunweg pushed a commit that referenced this pull request Aug 4, 2024
…getVert 1 (#15237)

This allows to leave out the not-nil hypothesis, in addition to not needing it for
the definition of tail.
bjoernkjoshanssen pushed a commit that referenced this pull request Sep 9, 2024
…getVert 1 (#15237)

This allows to leave out the not-nil hypothesis, in addition to not needing it for
the definition of tail.
bjoernkjoshanssen pushed a commit that referenced this pull request Sep 9, 2024
…getVert 1 (#15237)

This allows to leave out the not-nil hypothesis, in addition to not needing it for
the definition of tail.
bjoernkjoshanssen pushed a commit that referenced this pull request Sep 12, 2024
…getVert 1 (#15237)

This allows to leave out the not-nil hypothesis, in addition to not needing it for
the definition of tail.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

maintainer-merge A reviewer has approved the changed; awaiting maintainer approval. ready-to-merge This PR has been sent to bors. t-combinatorics Combinatorics

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants