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): merge init_.algebra into algebra #2707

Closed
wants to merge 52 commits into from

Conversation

digama0
Copy link
Member

@digama0 digama0 commented May 17, 2020

This is a big refactor PR. It depends on #2697, which brings in the algebra hierarchy without any change to the file structure. This PR merges init_.algebra.group into algebra.group and so on for the rest of the algebraic hierarchy.

@digama0 digama0 added the WIP Work in progress label May 17, 2020
src/algebra/group/basic.lean Outdated Show resolved Hide resolved
@urkud
Copy link
Member

urkud commented May 18, 2020

Absolutely not. It is crucial that algebra.group not depend on a lot of files because it is used everywhere, and adding more group theory like things makes it much more tempting to add more dependencies.

E.g., we can delete algebra.group.default, then adding more files into algebra/group will not automatically import them everywhere.

@digama0
Copy link
Member Author

digama0 commented May 18, 2020

I think that having more files in algebra/group, that are not imported with algebra.group and have unusually strong dependencies compared to other things in algebra is confusing. We currently have very few actual dependency barriers in mathlib, algebra is one of the few recognizable ones, and as a result it is very difficult to get guidance on what is supposed to depend on what and when adding an import is okay. It also means that most parts of mathlib have a huge number of unnecessary dependencies and take an hour to build.

So I'm really not happy about the idea of relaxing restrictions about low dependencies of algebra. If anything, I want to break it (and everything else) into more and smaller pieces, in particular separating theorems from definitions so that you can e.g. get real numbers without the binomial theorem.

@jcommelin
Copy link
Member

jcommelin commented May 18, 2020

We could rename algebra to algebraic_hierarchy. Then it's purpose is clear to everyone. (And we should kick some stuff out of that directory.)
The current name is very confusing to everyone that doesn't know that algebra is supposed to be minimal, and only developing the basic hierarchy.

@@ -1,45 +1,222 @@
/-
Copyright (c) 2017 Johannes Hölzl. All rights reserved.
Copyright (c) 2014 Robert Lewis. All rights reserved.
Copy link
Member

Choose a reason for hiding this comment

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

This copyright date does not seem to be accurate.

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 think most mathlib files contain the date of the first revision. The name and date here is from the init.algebra.field file. If we want to change this to (c) 2014-2020 or something then that should be applied to a lot more theorems than this.

instance division_ring.to_group_with_zero {K : Type*} [division_ring K] :
group_with_zero K :=
instance division_ring.to_group_with_zero :
group_with_zero α :=
Copy link
Member

Choose a reason for hiding this comment

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

I thought we're moving towards calling fields K instead of α?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is a group with zero a field? One thing I don't like about the new convention is that I am never sure what letter to use for these exotic structures. Anyway we can follow this up with some more cleanup in this vein.

Copy link
Member

Choose a reason for hiding this comment

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

No, but a division ring is a kind of field. A Schiefkörper, to be precise. So obviously it should get a K letter.

@@ -278,7 +278,7 @@ lemma f_squared : ∀ v : V n, (f n) (f n v) = (n : ℝ) • v :=
begin
induction n with n IH; intro,
{ simpa only [nat.cast_zero, zero_smul] },
{ cases v, simp [f_succ_apply, IH, add_smul], abel }
{ cases v, simp [f_succ_apply, IH, add_smul, add_assoc], abel }
Copy link
Member

Choose a reason for hiding this comment

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

Is add_assoc no longer simp?

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 was never supposed to be. It was officially removed as a simp lemma at the same time as mul_assoc, but due to a bug in the core version of to_additive it was getting marked as a simp lemma anyway.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I didn't notice. I only remember the add_comm/mul_comm story, where clearly neither should be simp lemmas. Is there a good reason *_assoc should not be simp?

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'm not totally clear on the reasons for removing these in the first place, but I think that consistency is good. Quite clearly even add_comm is okay for simp because it has explicit support for AC rewriting. Maybe these lemmas should be added to a simp set like simp with ac?

@@ -1,119 +1,294 @@
/-
Copyright (c) 2017 Mario Carneiro. All rights reserved.
Copyright (c) 2016 Jeremy Avigad. All rights reserved.
Copy link
Member

Choose a reason for hiding this comment

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

This copyright date also changes in the wrong direction.

@jcommelin
Copy link
Member

Thanks a megaton of 🐙 🎉 🍰 🎆

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 May 18, 2020
bors bot pushed a commit that referenced this pull request May 18, 2020
This is a big refactor PR. It depends on #2697, which brings in the algebra hierarchy without any change to the file structure. This PR merges `init_.algebra.group` into `algebra.group` and so on for the rest of the algebraic hierarchy.

Co-authored-by: Mario Carneiro <di.gama@gmail.com>
Co-authored-by: Bryan Gin-ge Chen <bryangingechen@gmail.com>
@bors
Copy link

bors bot commented May 18, 2020

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title refactor(algebra): merge init_.algebra into algebra [Merged by Bors] - refactor(algebra): merge init_.algebra into algebra May 18, 2020
@bors bors bot closed this May 18, 2020
@bors bors bot deleted the merge-algebra branch May 18, 2020 15:26
cipher1024 pushed a commit to cipher1024/mathlib that referenced this pull request Mar 15, 2022
…nity#2707)

This is a big refactor PR. It depends on leanprover-community#2697, which brings in the algebra hierarchy without any change to the file structure. This PR merges `init_.algebra.group` into `algebra.group` and so on for the rest of the algebraic hierarchy.

Co-authored-by: Mario Carneiro <di.gama@gmail.com>
Co-authored-by: Bryan Gin-ge Chen <bryangingechen@gmail.com>
YaelDillies added a commit to leanprover-community/mathlib4 that referenced this pull request Feb 5, 2024
Reorder the `WithOne` material. `Algebra.Group.WithOne.Defs` was hiding a `Ring` import! I credit Johan and Mario for leanprover-community/mathlib#2707.
mathlib-bors bot pushed a commit to leanprover-community/mathlib4 that referenced this pull request Feb 6, 2024
Reorder the `WithOne` material.
* `Algebra.Group.WithOne.Defs` was hiding a `Ring` import! I credit Johan and Mario for leanprover-community/mathlib#2707.
* `WithBot` is not needed to define `WithOne.unone`. It's simpler to redefine it by hand. In the future, we might want to have an `Option` version (but not sure how much that's worth, since it would basically be `Option.get` again).
atarnoam pushed a commit to leanprover-community/mathlib4 that referenced this pull request Feb 9, 2024
Reorder the `WithOne` material.
* `Algebra.Group.WithOne.Defs` was hiding a `Ring` import! I credit Johan and Mario for leanprover-community/mathlib#2707.
* `WithBot` is not needed to define `WithOne.unone`. It's simpler to redefine it by hand. In the future, we might want to have an `Option` version (but not sure how much that's worth, since it would basically be `Option.get` again).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

None yet

6 participants