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

refactor(algebra/group): make multiplicative and additive irreducible #2292

Closed
wants to merge 4 commits into from

Conversation

urkud
Copy link
Member

@urkud urkud commented Mar 30, 2020


Co-Authored-By: Chris Hughes <33847686+ChrisHughes24@users.noreply.github.com>
@@ -13,14 +13,54 @@ variables {α : Type u} {β : Type v}
def additive (α : Type*) := α
def multiplicative (α : Type*) := α
Copy link
Member

Choose a reason for hiding this comment

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

Does @[to_additive additive] work here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Why would we want it?

Copy link
Member

Choose a reason for hiding this comment

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

It seems less duplication to only define it once and have to_additive copy it, but maybe I'm overestimating the savings here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I mean, do we want to_additive to turn multiplicative in a lemma into additive? I don't think so. More precisely, I don't think that to_additive will work with a lemma involving both α and multiplicative β.

def to_add (x : α) : additive α := x

/-- Reinterpret `x : additive α` as an element of `α`. -/
def of_add (x : additive α) : α := x
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be in the additive namespace so that field notation works.

Copy link
Member Author

Choose a reason for hiding this comment

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

What rename do you propose?

Copy link
Member

Choose a reason for hiding this comment

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

additive.to_mul is the best that I can think of now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just to be sure that I understand correctly: do you propose __root__.to_mul, additive.to_mul, __root__.to_add and multiplicative.to_add? If we want to avoid polluting the __root__ namespace, we can use additive.of_mul, additive.to_mul, multiplicative.of_add and multiplicative.to_add.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think @gebner is worried about pollution, but rather wants to enable writing x.to_mul for x : additive foo.

Copy link
Member

Choose a reason for hiding this comment

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

@jcommelin That's right. This is the definition I'm proposing:

def additive.to_mul (x : additive α) : α := x

Copy link
Member Author

Choose a reason for hiding this comment

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

Then we should move to_mul to multiplicative.of_add because otherwise we'll have __root__.to_mul and additive.to_mul, and it will be hard to tell whether we go α → multiplicative α or additive α → α.

I think that I'll make a version of this PR that doesn't add irreducible but adds to_mul and of_mul + simp lemmas.

Copy link
Member

Choose a reason for hiding this comment

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

The naming of this function was just a suggestion. I still think that this PR makes the right choice in making multiplicative and additive irreducible.

Copy link
Member Author

Choose a reason for hiding this comment

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

It leads to quite a few build failures. I want to merge to_mul/of_mul/to_add/of_add before fixing these failures.

@urkud urkud added the WIP Work in progress label Apr 2, 2020
@github-actions github-actions bot added the merge-conflict Please `git merge origin/master` then a bot will remove this label. label Aug 13, 2020
@eric-wieser
Copy link
Member

Since #2363 is in, can this be revived?

@urkud
Copy link
Member Author

urkud commented Oct 30, 2020

Since #2363 is in, can this be revived?

There are some parts of mathlib (e.g., free_group) that rely too much on these type tags being semireducible.

@github-actions github-actions bot removed the merge-conflict Please `git merge origin/master` then a bot will remove this label. label Feb 4, 2021
@github-actions github-actions bot added the merge-conflict Please `git merge origin/master` then a bot will remove this label. label Mar 22, 2021
@urkud urkud closed this Oct 2, 2021
@urkud urkud deleted the mult-irreducible branch October 2, 2021 04:03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
merge-conflict Please `git merge origin/master` then a bot will remove this label. WIP Work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants