Skip to content

[Merged by Bors] - feat: simplification lemmas for Vector.map / Vector.mapAccumr#5558

Closed
alexkeizer wants to merge 17 commits intomasterfrom
vector-normmap
Closed

[Merged by Bors] - feat: simplification lemmas for Vector.map / Vector.mapAccumr#5558
alexkeizer wants to merge 17 commits intomasterfrom
vector-normmap

Conversation

@alexkeizer
Copy link
Copy Markdown
Collaborator

Primarily focused on folding nested applications of mapAccumr into a single mapAccumr


Open in Gitpod

@alexkeizer alexkeizer added awaiting-review awaiting-CI This PR does not pass CI yet. This label is automatically removed once it does. labels Jun 28, 2023
@github-actions github-actions bot removed the awaiting-CI This PR does not pass CI yet. This label is automatically removed once it does. label Jun 28, 2023
@alexkeizer alexkeizer requested a review from ChrisHughes24 June 28, 2023 20:13
Copy link
Copy Markdown
Member

@ChrisHughes24 ChrisHughes24 left a comment

Choose a reason for hiding this comment

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

One thing I'd like to see is that any theorem about Vectors that's proved here should also be proven for Lists, and maybe the Vector version should be a trivial consequence of the List version wherever possible.

Comment on lines +22 to +23
theorem map_to_mapAccumr (xs : Vector α n) (f : α → β) :
map f xs = (mapAccumr (fun x _ => (⟨⟩, f x)) xs ()).snd := by
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.

I think the equality should be the other way around and named something like mapAccumr_eq_map. Also I'm guessing the ⟨⟩ is type Unit, but I think a similar lemma applies for arbitrary types, as long as we don't depend on 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.

You are right. Actually, the reverse lemma is already present as mapAccumr_redundant_state at the bottom of the file. I'll remove these two

@[simp]
theorem mapAccumr_mapAccumr :
mapAccumr f₁ (mapAccumr f₂ xs s₂).snd s₁
= let m := (mapAccumr (fun x s =>
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.

I'm not a massive fan of lets in theorem statements, you almost always want to reduce them even if the resulting term is longer.

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.

Hmm, here the let is on the right side of the equation, so I thought it would be fine; these lemmas are intended as rewrite rules, I don't expect a goal of exactly this type to come up. Then again, that does mean we are making the reverse rule <-mapAccumr_mapAccumr more difficult to use.

The reduction is a bit nasty: here is a before/after

  let m := (mapAccumr (fun x s =>
    let r₂ := f₂ x s.snd
    let r₁ := f₁ r₂.snd s.fst
    ((r₁.fst, r₂.fst), r₁.snd)
  ) xs (s₁, s₂))
  (m.fst.fst, m.snd) 

Turns into

(
  (mapAccumr (fun x s => (
    ((f₁ (f₂ x s.snd).snd s.fst).fst, (f₂ x s.snd).fst), (f₁ (f₂ x s.snd).snd s.fst).snd)
  ) xs (s₁, s₂)).fst.fst, 
  (mapAccumr (fun x s => (
    ((f₁ (f₂ x s.snd).snd s.fst).fst, (f₂ x s.snd).fst), (f₁ (f₂ x s.snd).snd s.fst).snd)
  ) xs (s₁, s₂)).snd
) 

I'll defer to you on whether the readability penalty is worth it

induction xs using Vector.revInductionOn generalizing s <;> simp_all

@[simp]
theorem map_map (f₁ : β → γ) (f₂ : α → β) :
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.

Is this not a duplication? It seems like it would already be there.

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.

Neither apply? nor simp seem to find any such existing lemma, so I don't think we have it

pair, then we can simplify to just a single element of state
-/
@[simp]
theorem mapAccumr_redundant_pair (f : α → (σ × σ) → (σ × σ) × β)
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.

I feel like this theorem can be generalized. Are there more general conditions where you can replace the state type with a different state type, that proves this lemma as a special case?

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.

It probably can be, I'll have to think about it a bit more. I choose products in particular, because the earlier folding of nested mapAccumrs has a tendency to produce pairs with redundant state, so this theorem is needed to counterbalance 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.

The most general way to relate two applications of mapAccumr, with potentially different states, is bisimulation. I've added bisimulation lemmas, and proven this result in terms of bisimulation

Comment on lines +264 to +266
theorem mapAccumr₂_unused_input_left [Inhabited α] (f : α → β → σ → σ × γ)
(h : ∀ a b s, f default b s = f a b s) :
mapAccumr₂ f xs ys s = mapAccumr (fun b s => f default b s) ys s := by
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.

I'm not sure that this is a good simp lemma because we have a hypothesis.

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.

As in, it won't work, or it's not considered appropriate?
I think it does work, but I can always add it to just the aesop ruleset, instead of as a simp-lemma

@alexkeizer
Copy link
Copy Markdown
Collaborator Author

One thing I'd like to see is that any theorem about Vectors that's proved here should also be proven for Lists, and maybe the Vector version should be a trivial consequence of the List version wherever possible.

I agree that these results shouldn't necessarily be just for Vectors, but I'm not sure moving them to List and duplicating is necessarily a good idea. Especially since there seems to be some wish to make Vector a subtype of Array rather than List. Ideally, I'd like to state them generically over some ListLike interface.

alexkeizer and others added 2 commits June 29, 2023 15:31
Co-authored-by: Chris Hughes <33847686+ChrisHughes24@users.noreply.github.com>
@ChrisHughes24
Copy link
Copy Markdown
Member

A ListLike class seems like a lot of work and certainly outside the scope of this PR. I won't insist on any theorem duplication.

@alexkeizer alexkeizer added WIP Work in progress awaiting-author A reviewer has asked the author a question or requested changes. and removed awaiting-review labels Jun 30, 2023
@alexkeizer alexkeizer added awaiting-review and removed awaiting-author A reviewer has asked the author a question or requested changes. WIP Work in progress awaiting-review labels Jul 6, 2023
Comment on lines +141 to +142
theorem mapAccumr₂_mapAccumr₂_right_left (f₁ : α → γ → σ₁ → σ₁ × φ)
(f₂ : α → β → σ₂ → σ₂ × γ) :
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.

Suggested change
theorem mapAccumr₂_mapAccumr₂_right_left (f₁ : α → γ → σ₁ → σ₁ × φ)
(f₂ : α → β → σ₂ → σ₂ × γ) :
theorem mapAccumr₂_mapAccumr₂_right_left
(f₁ : α → γ → σ₁ → σ₁ × φ) (f₂ : α → β → σ₂ → σ₂ × γ) :

Comment on lines +154 to +155
theorem mapAccumr₂_mapAccumr₂_right_right (f₁ : β → γ → σ₁ → σ₁ × φ)
(f₂ : α → β → σ₂ → σ₂ × γ) :
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.

Suggested change
theorem mapAccumr₂_mapAccumr₂_right_right (f₁ : β → γ → σ₁ → σ₁ × φ)
(f₂ : α → β → σ₂ → σ₂ × γ) :
theorem mapAccumr₂_mapAccumr₂_right_right
(f₁ : β → γ → σ₁ → σ₁ × φ) (f₂ : α → β → σ₂ → σ₂ × γ) :


@[simp]
theorem mapAccumr₂_mapAccumr₂_left_right
(f₁ : γ → β → σ₁ → σ₁ × φ) (f₂ : α → β → σ₂ → σ₂ × γ) :
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.

Suggested change
(f₁ : γ → β → σ₁ → σ₁ × φ) (f₂ : α → β → σ₂ → σ₂ × γ) :
(f₁ : γ → β → σ₁ → σ₁ × φ) (f₂ : α → β → σ₂ → σ₂ × γ) :

induction xs, ys using Vector.revInductionOn₂ <;> simp_all

@[simp]
theorem mapAccumr₂_mapAccumr_right (f₁ : α → γ → σ₁ → σ₁ × ζ) (f₂ : β → σ₂ → σ₂ × γ) :
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.

I think I might have changed my mind about the use of lets. It's very unreadable right now. Maybe the lets should be reintroduced on second thoughts.

@ChrisHughes24
Copy link
Copy Markdown
Member

bors merge

@github-actions github-actions bot added the ready-to-merge This PR has been sent to bors. label Jul 10, 2023
bors bot pushed a commit that referenced this pull request Jul 10, 2023
Primarily focused on folding nested applications of `mapAccumr` into a single `mapAccumr`



Co-authored-by: Chris Hughes <chrishughes24@gmail.com>
@bors
Copy link
Copy Markdown

bors bot commented Jul 10, 2023

Pull request successfully merged into master.

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot changed the title feat: simplification lemmas for Vector.map / Vector.mapAccumr [Merged by Bors] - feat: simplification lemmas for Vector.map / Vector.mapAccumr Jul 10, 2023
@bors bors bot closed this Jul 10, 2023
@bors bors bot deleted the vector-normmap branch July 10, 2023 12:17
kim-em pushed a commit that referenced this pull request Aug 14, 2023
Primarily focused on folding nested applications of `mapAccumr` into a single `mapAccumr`



Co-authored-by: Chris Hughes <chrishughes24@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-to-merge This PR has been sent to bors.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants