Skip to content
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

[Merged by Bors] - refactor(algebra/ring_quot.lean): make ring_quot irreducible #7120

Closed
wants to merge 6 commits into from

Conversation

sgouezel
Copy link
Collaborator

@sgouezel sgouezel commented Apr 8, 2021

The quotient of a ring by an equivalence relation which is compatible with the operations is still a ring. This is used to define several objects such as tensor algebras, exterior algebras, and so on, but the details of the construction are irrelevant. This PR makes ring_quot irreducible to keep the complexity down.

@@ -71,7 +71,7 @@ by simp [reverse]
by simp [reverse]

@[simp] lemma reverse.map_one : reverse (1 : clifford_algebra Q) = 1 :=
reverse.commutes 1
by convert reverse.commutes (1 : R); simp
Copy link
Member

Choose a reason for hiding this comment

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

This scares me a little - it sounds like algebra_map R 1 (clifford_algebra Q) is no longer defeq to 1, which seems like a useful property to have.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

defeqness is nice when you can have it, but not really important IMHO: there are many situations where algebra_map R 1 S is not defeq to 1, but it is not a problem since you can rewrite it away.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For instance, I just checked and algebra_map ℕ ℝ 1 = 1 is not definitional.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, but that's because the reals just underwent the same refactor! From reading the source alone, I think that both matrix_algebra and triv_sq_zero_ext satisfy this.

@eric-wieser
Copy link
Member

eric-wieser commented Apr 8, 2021

This is used to define several objects such as tensor algebras, exterior algebras, and so on, but the details of the construction are irrelevant.

While this is true, in principle ring_quot could be used for any of the things ideal.quotient is currently used for today, right? If there are reasons that it's useful for ideal.quotient to be reducible, presumably those translate here.

On the other hand, the performance is obviously a real issue now, but the definitional transparency is at worst a slight inconvenience,

@sgouezel
Copy link
Collaborator Author

sgouezel commented Apr 8, 2021

I'd say ideal.quotient should also be irreducible for the same reasons.

@eric-wieser
Copy link
Member

@Vierkantor, what was the original motivation for the refactor that made div no longer defeq to a combination of mul and inv? Was definitional equality relevant at all? I'm curious if the rationale there is relevant here.

@github-actions github-actions bot added the merge-conflict Please `git merge origin/master` then a bot will remove this label. label Apr 8, 2021
@Vierkantor
Copy link
Collaborator

@Vierkantor, what was the original motivation for the refactor that made div no longer defeq to a combination of mul and inv? Was definitional equality relevant at all? I'm curious if the rationale there is relevant here.

The motivation was the following: the previous has_div instance deriving from group_with_zero was defeq-incompatible with the has_div instance on fractional_ideal. Under certain conditions you want group_with_zero (fractional_ideal _), so you need to be able to specify the value of has_div.div arising from group_with_zero, otherwise you get non-defeq diamond instances.

@Vierkantor
Copy link
Collaborator

For this PR, I'm willing to accept the non-defeqness, since it looks like you still have that one expression 1 : clifford_algebra Q is always defeq to another 1 : clifford_algebra Q.

@eric-wieser
Copy link
Member

Ah, so the previous problem was a diamond which required control of definitional equality, whereas here there's no diamond to introduce.

@github-actions github-actions bot removed the merge-conflict Please `git merge origin/master` then a bot will remove this label. label Apr 9, 2021
@sgouezel sgouezel added the awaiting-review The author would like community review of the PR label Apr 13, 2021
@github-actions github-actions bot added the merge-conflict Please `git merge origin/master` then a bot will remove this label. label Apr 14, 2021
@github-actions github-actions bot removed the merge-conflict Please `git merge origin/master` then a bot will remove this label. label Apr 21, 2021
Copy link
Collaborator

@Vierkantor Vierkantor left a comment

Choose a reason for hiding this comment

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

Looks good to me, and let's get this merged before it breaks again.

bors d+

src/algebra/ring_quot.lean Outdated Show resolved Hide resolved
@bors
Copy link

bors bot commented Apr 22, 2021

✌️ sgouezel can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

Co-authored-by: Anne Baanen <Vierkantor@users.noreply.github.com>
@sgouezel
Copy link
Collaborator Author

bors r+

bors bot pushed a commit that referenced this pull request Apr 22, 2021
The quotient of a ring by an equivalence relation which is compatible with the operations is still a ring. This is used to define several objects such as tensor algebras, exterior algebras, and so on, but the details of the construction are irrelevant. This PR makes `ring_quot` irreducible to keep the complexity down.
@bors
Copy link

bors bot commented Apr 22, 2021

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title refactor(algebra/ring_quot.lean): make ring_quot irreducible [Merged by Bors] - refactor(algebra/ring_quot.lean): make ring_quot irreducible Apr 22, 2021
@bors bors bot closed this Apr 22, 2021
@bors bors bot deleted the ring_quot_irreduc branch April 22, 2021 20:22
@YaelDillies YaelDillies removed the awaiting-review The author would like community review of the PR label Nov 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants