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] - chore: refactor of Algebra/Group/Defs to reduce imports #9606

Closed
wants to merge 16 commits into from

Conversation

semorrison
Copy link
Contributor

@semorrison semorrison commented Jan 10, 2024

We are not that far from the point that Algebra/Group/Defs depends on nothing significant besides simps and to_additive.

This removes from Mathlib.Algebra.Group.Defs the dependencies on

  • Mathlib.Tactic.Basic (which is a grab-bag of random stuff)
  • Mathlib.Init.Algebra.Classes (which is ancient and half-baked)
  • Mathlib.Logic.Function.Basic (not particularly important, but it is barely used in this file)

The goal is to avoid all unnecessary imports to set up the definitions of basic algebraic structures.

We also separate out Mathlib.Tactic.TypeStar and Mathlib.Tactic.Lemma as prerequisites to Mathlib.Tactic.Basic, but which can be imported separately when the rest of Mathlib.Tactic.Basic is not needed.


Open in Gitpod

@semorrison semorrison added the awaiting-review The author would like community review of the PR label Jan 10, 2024
@Vierkantor Vierkantor self-assigned this Jan 10, 2024
@YaelDillies YaelDillies self-assigned this Jan 10, 2024
@YaelDillies
Copy link
Collaborator

Can you

  • explain the motivation in the PR description
  • give a more precise description than "reduce imports". Here you're getting rid of the dependency on Injective

Personally I'm wondering why you're trying to get rid of Injective. I understand you would like to carve out an algebraic order hierarchy library, but

  • Injective could (should?) very well live ahead of that library since it could be in Std
  • later files in the algebraic order hierarchy really need some basics of Injective, Equiv...

Copy link
Contributor

@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.

Nice work!

For the benefit of future readers, could you also note the changes to Tactic.Basic in the PR description?

bors d=@YaelDillies

Mathlib/Algebra/Group/Classes.lean Outdated Show resolved Hide resolved
Mathlib/Tactic/Basic.lean Show resolved Hide resolved
Mathlib/CategoryTheory/Shift/Basic.lean Outdated Show resolved Hide resolved
Mathlib/Algebra/GroupWithZero/Defs.lean Outdated Show resolved Hide resolved
Mathlib/Algebra/Group/Semiconj/Defs.lean Outdated Show resolved Hide resolved
Mathlib/Algebra/Group/Defs.lean Show resolved Hide resolved
@mathlib-bors
Copy link

mathlib-bors bot commented Jan 10, 2024

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

@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added delegated and removed awaiting-review The author would like community review of the PR labels Jan 10, 2024
@Vierkantor Vierkantor removed their assignment Jan 10, 2024
Copy link
Collaborator

@YaelDillies YaelDillies left a comment

Choose a reason for hiding this comment

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

From my reading, you're not attached to having injectivity lemmas be in a new file on their own. Therefore could they go in Algebra.Group.Basic?

@semorrison
Copy link
Contributor Author

semorrison commented Jan 11, 2024

From my reading, you're not attached to having injectivity lemmas be in a new file on their own. Therefore could they go in Algebra.Group.Basic?

Done.

@semorrison semorrison added the awaiting-review The author would like community review of the PR label Jan 11, 2024
Mathlib/Algebra/Group/Classes.lean Outdated Show resolved Hide resolved
Mathlib/Algebra/Group/Classes.lean Outdated Show resolved Hide resolved
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added merge-conflict The PR has a merge conflict with master, and needs manual merging. and removed merge-conflict The PR has a merge conflict with master, and needs manual merging. labels Jan 13, 2024
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot removed the merge-conflict The PR has a merge conflict with master, and needs manual merging. label Jan 15, 2024
@YaelDillies
Copy link
Collaborator

bors merge

mathlib-bors bot pushed a commit that referenced this pull request Jan 17, 2024
We are not that far from the point that `Algebra/Group/Defs` depends on nothing significant besides `simps` and `to_additive`.

This removes from `Mathlib.Algebra.Group.Defs` the dependencies on
* `Mathlib.Tactic.Basic` (which is a grab-bag of random stuff)
* `Mathlib.Init.Algebra.Classes` (which is ancient and half-baked)
* `Mathlib.Logic.Function.Basic` (not particularly important, but it is barely used in this file)

The goal is to avoid all unnecessary imports to set up the *definitions* of basic algebraic structures. 

We also separate out `Mathlib.Tactic.TypeStar` and `Mathlib.Tactic.Lemma` as prerequisites to `Mathlib.Tactic.Basic`, but which can be imported separately when the rest of `Mathlib.Tactic.Basic` is not needed.



Co-authored-by: Scott Morrison <scott.morrison@gmail.com>
Co-authored-by: Yaël Dillies <yael.dillies@gmail.com>
@mathlib-bors
Copy link

mathlib-bors bot commented Jan 17, 2024

Pull request successfully merged into master.

Build succeeded:

@mathlib-bors mathlib-bors bot changed the title chore: refactor of Algebra/Group/Defs to reduce imports [Merged by Bors] - chore: refactor of Algebra/Group/Defs to reduce imports Jan 17, 2024
@mathlib-bors mathlib-bors bot closed this Jan 17, 2024
@mathlib-bors mathlib-bors bot deleted the group_defs_refactor branch January 17, 2024 13:26
Comment on lines +12 to +25
open Lean

/-- `lemma` means the same as `theorem`. It is used to denote "less important" theorems -/
syntax (name := lemma) declModifiers
group("lemma " declId ppIndent(declSig) declVal Parser.Command.terminationSuffix) : command

/-- Implementation of the `lemma` command, by macro expansion to `theorem`. -/
@[macro «lemma»] def expandLemma : Macro := fun stx =>
-- FIXME: this should be a macro match, but terminationSuffix is not easy to bind correctly.
-- This implementation ensures that any future changes to `theorem` are reflected in `lemma`
let stx := stx.modifyArg 1 fun stx =>
let stx := stx.modifyArg 0 (mkAtomFrom · "theorem" (canonical := true))
stx.setKind ``Parser.Command.theorem
pure <| stx.setKind ``Parser.Command.declaration
Copy link
Member

@eric-wieser eric-wieser Mar 4, 2024

Choose a reason for hiding this comment

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

@semorrison, was moving these declarations to the root namespace (instead of leaving them in Mathlib.Tactic) deliberate?

@sgouezel sgouezel removed the awaiting-review The author would like community review of the PR label Mar 12, 2024
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

6 participants