Skip to content

fix: unsqueeze simp, re Yaël's comments on #11259#11455

Open
adomani wants to merge 3 commits intomasterfrom
adomani/yael_unsqueeze_simp
Open

fix: unsqueeze simp, re Yaël's comments on #11259#11455
adomani wants to merge 3 commits intomasterfrom
adomani/yael_unsqueeze_simp

Conversation

@adomani
Copy link
Copy Markdown
Contributor

@adomani adomani commented Mar 17, 2024

This PR reverts/simplifies some of the "squeeze simp" changes in #11259.

See #11259 for context.


Open in Gitpod

Co-authored-by: Ruben Van de Velde <65514131+Ruben-VandeVelde@users.noreply.github.com>
@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 Mar 20, 2024
@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 Mar 22, 2024
theorem head_ofSeq (s : Seq α) : head (ofSeq s) = Computation.pure s.head := by
simp only [head, Option.map_eq_map, destruct_ofSeq, Computation.map_pure, Option.map_map]
cases Seq.head s <;> rfl
simp [head]; cases Seq.head s <;> rfl
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@YaelDillies Why do you want to bring back a non-terminal simp here? If you don't like simp only, then we can use simp? says simp only ...

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.

Here is my comment for context: #11259 (comment)

My arguments for the squeezed simp being a worse proof are that

  1. It mentions more lemmas
  2. Those lemmas are part of the very basic API of their corresponding defs and could change if the defs change defeq
  3. The non-terminal simp was just unfolding the definitions anyway (importantly, in a way that doesn't depend on the exact defeqs)

@loefflerd
Copy link
Copy Markdown
Contributor

This PR has been on the queue for a long time now, we should make a decision.

Yael: I gather you're against the simp only because it mentions too many low-level lemmas which might change. So how about avoiding a non-terminal simp by using the suffices new_goal by simp idiom?

@robertylewis
Copy link
Copy Markdown
Member

robertylewis commented Jun 19, 2024

I agree that reintroducing the nonterminal simp isn't right here. If any change is wanted here,

So how about avoiding a non-terminal simp by using the suffices new_goal by simp idiom?

sounds good to me.

@YaelDillies
Copy link
Copy Markdown
Contributor

Sure, we can try. I'm not yet convinced it's better than having rfl following simp, though

@loefflerd loefflerd added awaiting-author A reviewer has asked the author a question or requested changes. and removed awaiting-review labels Jun 21, 2024
theorem head_ofSeq (s : Seq α) : head (ofSeq s) = Computation.pure s.head := by
simp only [head, Option.map_eq_map, destruct_ofSeq, Computation.map_pure, Option.map_map]
cases Seq.head s <;> rfl
simp [head]; cases Seq.head s <;> rfl
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.

Suggested change
simp [head]; cases Seq.head s <;> rfl
suffices Computation.pure (Option.map (Prod.fst ∘ (·, s.tail)) s.head)
= Computation.pure (s.head) by simpa [head]
cases Seq.head s <;> rfl

@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 19, 2024
@grunweg
Copy link
Copy Markdown
Contributor

grunweg commented Apr 15, 2025

Coming here from triaging stale PRs: is this PR still relevant? If not, feel free to close it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting-author A reviewer has asked the author a question or requested changes. merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants