Skip to content

perf(Algebra/Ring): faster Ring.toAddGroupWithOne instance#38017

Open
kbuzzard wants to merge 7 commits intoleanprover-community:masterfrom
kbuzzard:kbuzzard-ring-to-addgroupwithone
Open

perf(Algebra/Ring): faster Ring.toAddGroupWithOne instance#38017
kbuzzard wants to merge 7 commits intoleanprover-community:masterfrom
kbuzzard:kbuzzard-ring-to-addgroupwithone

Conversation

@kbuzzard
Copy link
Copy Markdown
Member

@kbuzzard kbuzzard commented Apr 13, 2026

An attempt to manually make Ring.toAddGroupWithOne a fast instance.


Open in Gitpod

#mathlib4 > instance unfolding phenomenon @ 💬

@kbuzzard kbuzzard added the WIP Work in progress label Apr 13, 2026
@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 13, 2026

PR summary 9f9c40f019

Import changes for modified files

Dependency changes

File Base Count Head Count Change
Mathlib.Algebra.Ring.Defs 92 93 +1 (+1.09%)
Mathlib.Algebra.Field.Defs 95 96 +1 (+1.05%)
Import changes for all files
Files Import difference
46 files Mathlib.Algebra.ContinuedFractions.Basic Mathlib.Algebra.EuclideanDomain.Defs Mathlib.Algebra.EuclideanDomain.Field Mathlib.Algebra.EuclideanDomain.Int Mathlib.Algebra.Field.Defs Mathlib.Algebra.Field.Equiv Mathlib.Algebra.Field.IsField Mathlib.Algebra.Field.MinimalAxioms Mathlib.Algebra.Homology.Embedding.Basic Mathlib.Algebra.Module.Defs Mathlib.Algebra.Module.MinimalAxioms Mathlib.Algebra.Module.Prod Mathlib.Algebra.Order.Hom.Basic Mathlib.Algebra.Order.Module.Synonym Mathlib.Algebra.Order.Ring.Idempotent Mathlib.Algebra.Order.Ring.Synonym Mathlib.Algebra.Order.Ring.Unbundled.Rat Mathlib.Algebra.Ring.Action.Rat Mathlib.Algebra.Ring.Centralizer Mathlib.Algebra.Ring.Defs Mathlib.Algebra.Ring.Divisibility.Basic Mathlib.Algebra.Ring.Ext Mathlib.Algebra.Ring.GrindInstances Mathlib.Algebra.Ring.Hom.InjSurj Mathlib.Algebra.Ring.Idempotent Mathlib.Algebra.Ring.InjSurj Mathlib.Algebra.Ring.Int.Defs Mathlib.Algebra.Ring.Invertible Mathlib.Algebra.Ring.MinimalAxioms Mathlib.Algebra.Ring.Nat Mathlib.Algebra.Ring.PUnit Mathlib.Algebra.Ring.Regular Mathlib.Algebra.Ring.Semiconj Mathlib.Algebra.Ring.WithZero Mathlib.Algebra.Tropical.Basic Mathlib.Data.Int.Cast.Field Mathlib.Data.Int.LeastGreatest Mathlib.Data.Int.NatAbs Mathlib.Data.Nat.Cast.WithTop Mathlib.Data.Nat.WithBot Mathlib.GroupTheory.GroupAction.Ring Mathlib.RingTheory.LocalRing.Defs Mathlib.SetTheory.Lists Mathlib.Tactic.NormNum.Core Mathlib.Tactic.NormNum.Parity Mathlib.Tactic.NormNum.Result
1

Declarations diff

+ AddGroupWithOne.toAddGroup'
+ AddGroupWithOne.toAddMonoidWithOne'
+ AddGroupWithOne.toSubNegMonoid
+ NonUnitalCommRing.toNonUnitalNonAssocCommRing'
+ Ring.toAddCommGroup'
+ Ring.toAddGroupWithOne'
+ instance (priority := 100) Field.toSemifield [Field K] : Semifield K
- instance (priority := 100) Field.toSemifield [Field K] : Semifield K := { ‹Field K› with }

You can run this locally as follows
## summary with just the declaration names:
./scripts/pr_summary/declarations_diff.sh <optional_commit>

## more verbose report:
./scripts/pr_summary/declarations_diff.sh long <optional_commit>

The doc-module for scripts/pr_summary/declarations_diff.sh contains some details about this script.


No changes to technical debt.

You can run this locally as

./scripts/reporting/technical-debt-metrics.sh pr_summary
  • The relative value is the weighted sum of the differences with weight given by the inverse of the current value of the statistic.
  • The absolute value is the relative value divided by the total sum of the inverses of the current values (i.e. the weighted average of the differences).

@github-actions github-actions bot added the t-algebra Algebra (groups, rings, fields, etc) label Apr 13, 2026
@kbuzzard
Copy link
Copy Markdown
Member Author

!radar

@leanprover-radar
Copy link
Copy Markdown

leanprover-radar commented Apr 13, 2026

Benchmark results for 9a7e8db against 00dbc4e are in. There are significant results. @kbuzzard

  • 🟥 main exited with code 1

No significant changes detected.

/-- A `Ring` is a `Semiring` with negation making it an additive group. -/
class Ring (R : Type u) extends Semiring R, AddCommGroup R, AddGroupWithOne R

attribute [-instance] Ring.toAddGroupWithOne
Copy link
Copy Markdown
Member

@eric-wieser eric-wieser Apr 13, 2026

Choose a reason for hiding this comment

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

This only disables it for this file / section, it doesn't propagate to downstream files.

@@ -145,6 +145,13 @@ class Semiring (α : Type u) extends NonUnitalSemiring α, NonAssocSemiring α,
/-- A `Ring` is a `Semiring` with negation making it an additive group. -/
class Ring (R : Type u) extends Semiring R, AddCommGroup R, AddGroupWithOne R
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You might have better luck with

Suggested change
class Ring (R : Type u) extends Semiring R, AddCommGroup R, AddGroupWithOne R
class Ring (R : Type u) extends Semiring R, AddCommGroupWithOne R

and removing your other changes in this file.

@github-actions github-actions bot added the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Apr 14, 2026
@github-actions github-actions bot removed the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Apr 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

t-algebra Algebra (groups, rings, fields, etc) WIP Work in progress

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants