Skip to content
This repository has been archived by the owner on Jul 24, 2024. It is now read-only.

[Merged by Bors] - chore(*): remove after the fact attribute [irreducible] at several places #18168

Closed
wants to merge 16 commits into from

Conversation

sgouezel
Copy link
Collaborator

Part of #18164


Open in Gitpod

@sgouezel sgouezel added awaiting-review The author would like community review of the PR awaiting-CI The author would like to see what CI has to say before doing more work. labels Jan 13, 2023
@sgouezel sgouezel added WIP Work in progress and removed awaiting-review The author would like community review of the PR labels Jan 13, 2023
@sgouezel sgouezel requested a review from a team as a code owner January 13, 2023 18:11
@sgouezel sgouezel added the awaiting-review The author would like community review of the PR label Jan 13, 2023
Comment on lines 270 to 274
@[simp]
theorem lift_ι_apply (f : X → A) (x) :
lift R f (ι R x) = f x := rfl
lift R f (ι R x) = f x :=
by { rw [ι, lift], refl }

Copy link
Member

Choose a reason for hiding this comment

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

This is pretty unfortunate: having lift R f (ι R x) = f true feels like having sum.elim f g (sum.inl x) = f x true definitionally, in that it's effectively the recursor for the type.

This is expecially jarrying ater #18121, which only just introduced this defeq for tensor_product.

What happens if we just don't make it irreducible at all?

Copy link
Collaborator Author

@sgouezel sgouezel Jan 14, 2023

Choose a reason for hiding this comment

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

In the current situation, iota is made irreducible a few lines below, so this lemma doesn't hold definitionally by the end of this file currently, and the PR doesn't change that.

We could also drop irreducibility of iota and lift, but I guess it was there for performance reasons. This would probably belong to a different PR, though.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, good point. I still think this is unfortunate, but now agree that's out of scope for this PR

Comment on lines 43 to 46
clifford_algebra.lift Q ⟨-(ι Q), λ m, by simp⟩
begin
refine clifford_algebra.lift Q _,
exact ⟨-(ι Q), λ m, by simp only [linear_map.neg_apply, mul_neg, neg_mul, ι_sq_scalar, neg_neg]⟩
end
Copy link
Member

@eric-wieser eric-wieser Jan 13, 2023

Choose a reason for hiding this comment

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

Did squeezing the simp not work without the separate refine, or is this just a stylistic choice? If the former, a comment would be good.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It didn't work without the separate refine when I removed the irreducible tag on clifford_algebra (I don't see how to make that irreducible in Lean 4). But since this is a can of worms, I have removed the Cliffod algebra stuff from this PR.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for trying!

@github-actions github-actions bot removed the awaiting-CI The author would like to see what CI has to say before doing more work. label Jan 14, 2023
@sgouezel sgouezel added mathport For compatibility with Lean 4 changes, to simplify porting and removed WIP Work in progress labels Jan 14, 2023
@fpvandoorn
Copy link
Member

Thanks!

bors merge

@github-actions github-actions bot added the ready-to-merge All that is left is for bors to build and merge this PR. (Remember you need to say `bors r+`.) label Jan 16, 2023
@github-actions github-actions bot removed the awaiting-review The author would like community review of the PR label Jan 16, 2023
bors bot pushed a commit that referenced this pull request Jan 16, 2023
bors bot pushed a commit that referenced this pull request Jan 16, 2023
@bors
Copy link

bors bot commented Jan 16, 2023

Build failed (retrying...):

bors bot pushed a commit that referenced this pull request Jan 16, 2023
bors bot pushed a commit that referenced this pull request Jan 16, 2023
@bors
Copy link

bors bot commented Jan 16, 2023

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title chore(*): remove after the fact attribute [irreducible] at several places [Merged by Bors] - chore(*): remove after the fact attribute [irreducible] at several places Jan 16, 2023
@bors bors bot closed this Jan 16, 2023
@bors bors bot deleted the SG_no_irreducible branch January 16, 2023 15:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
mathport For compatibility with Lean 4 changes, to simplify porting 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.

3 participants