Skip to content

refactor(Algebra/Order): unbundle group and ring cone - #37298

Open
artie2000 wants to merge 7 commits into
leanprover-community:masterfrom
artie2000:unbundle-cone
Open

refactor(Algebra/Order): unbundle group and ring cone#37298
artie2000 wants to merge 7 commits into
leanprover-community:masterfrom
artie2000:unbundle-cone

Conversation

@artie2000

@artie2000 artie2000 commented Mar 28, 2026

Copy link
Copy Markdown
Collaborator
  • Unbundle GroupCone and RingCone using Submonoid.IsMulPointed

The material in Mathlib.Algebra.Group.Submonoid.Support was created to treat uniformly

  • positive cones in groups and rings
  • pointed cones in vector spaces over an ordered field
  • orderings in rings

This PR deprecates the GroupCone and RingCone structures, making use of the predicates Submonoid.IsMulPointed and AddSubmonoid.IsPointed defined in that file instead.

See also #36863 for the analogous change to ring orderings.


Open in Gitpod

@github-actions github-actions Bot added the large-import Automatically added label for PRs with a significant increase in transitive imports label Mar 28, 2026
@github-actions

github-actions Bot commented Mar 28, 2026

Copy link
Copy Markdown

PR summary 35471d41e9

Import changes exceeding 2%

% File
+89.37% Mathlib.Algebra.Order.Group.Cone
+67.15% Mathlib.Algebra.Order.Ring.Cone

Import changes for modified files

Dependency changes

File Base Count Head Count Change
Mathlib.Algebra.Order.Group.Cone 348 659 +311 (+89.37%)
Mathlib.Algebra.Order.Ring.Cone 414 692 +278 (+67.15%)
Import changes for all files
Files Import difference
Mathlib.Algebra.Order.Ring.Cone 278
Mathlib.Algebra.Order.Group.Cone 311

Declarations diff

+ IsOrderedMonoid.mkOfSubmonoid
+ IsOrderedRing.mkOfSubsemiring
+ LinearOrder.mkOfSubmonoid
+ PartialOrder.mkOfSubmonoid
+ PartialOrder.mkOfSubmonoid_le_iff
+ Submonoid.oneLE.isMulPointed
+ Submonoid.oneLE.isMulSpanning
+ Subsemiring.nonneg.isPointed
+ Subsemiring.nonneg.isSpanning

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.


Increase in tech debt: (relative, absolute) = (3.00, 0.12)
Current number Change Type
25 3 disabled deprecation lints

Current commit 85c2dc228d
Reference commit 35471d41e9

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 Mar 28, 2026
@dagurtomas dagurtomas removed their assignment Apr 2, 2026
@joelriou joelriou removed their assignment Apr 6, 2026
@joneugster

Copy link
Copy Markdown
Contributor

Could you please extend the PR description explaining why this is desired or what thee benefits are? And if there is a relevant Zulip discussion, could you please link that, too?

Not sure I'm the right person to review. I'll unassign myself for now but keep this on my list to revisit again

@YaelDillies YaelDillies left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Are we never going to talk about suprema/infima of group cones?

Why unbundle group cones and not other types of subobjects. Is the point that group cones aren't subobjects?

@Vierkantor Vierkantor added the awaiting-zulip There is a Zulip discussion; the author should await and report/implement the decision reached there label May 29, 2026
@Vierkantor

Copy link
Copy Markdown
Contributor

I agree with Jon that we should discuss this on Zulip before more review.

@artie2000

Copy link
Copy Markdown
Collaborator Author

@YaelDillies the operations on them are just the operations on the underlying submonoids / subsemirings. They don't have a sup or sInf in general. I believe this is the exact reasoning you used to convince me to unbundle RingPreordering, which led to me writing unbundled group/ring cones (Submonoid.IsPointed) in the first place.

@artie2000

artie2000 commented May 31, 2026

Copy link
Copy Markdown
Collaborator Author

Before I stopped contributing for around a month, I had spent months trying to get engagement on discussing this design on Zulip, to little avail. I don't think it makes sense to have the bundled and the unbundled version at the same time, so we should pick one and stick with it. My understanding of the consensus for bundling/unbundling subobjects was that we should bundle when the objects form a lattice and unbundle when we can't define lattice operations on them.
https://leanprover.zulipchat.com/#narrow/channel/116395-maths/topic/Ring.20orderings.20-.20structure.20or.20predicate.3F/with/562594050

@artie2000 artie2000 removed the awaiting-zulip There is a Zulip discussion; the author should await and report/implement the decision reached there label May 31, 2026
@artie2000

Copy link
Copy Markdown
Collaborator Author

Removed awaiting-zulip because there is no active Zulip discussion and the previous discussion on this topic got little engagement.

Comment on lines +25 to +26
theorem Submonoid.oneLE.isMulPointed [PartialOrder G] [IsOrderedMonoid G] :
(oneLE G).IsMulPointed := by aesop (add simp ge_antisymm_iff)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
theorem Submonoid.oneLE.isMulPointed [PartialOrder G] [IsOrderedMonoid G] :
(oneLE G).IsMulPointed := by aesop (add simp ge_antisymm_iff)
theorem Submonoid.IsMulPointed.oneLE [PartialOrder G] [IsOrderedMonoid G] :
(oneLE G).IsMulPointed := by aesop (add simp ge_antisymm_iff)

Same below

/-- Construct a partial order by designating a submonoid with zero support in an abelian group. -/
@[to_additive
/-- Construct a partial order by designating a submonoid with zero support in an abelian group. -/]
abbrev PartialOrder.mkOfSubmonoid : PartialOrder G where

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
abbrev PartialOrder.mkOfSubmonoid : PartialOrder G where
abbrev PartialOrder.ofSubmonoid : PartialOrder G where

is clear enough IMO

Comment on lines +35 to +37
@[to_additive
/-- Construct a partial order by designating a submonoid with zero support in an abelian group. -/]
abbrev PartialOrder.mkOfSubmonoid : PartialOrder G where

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
@[to_additive
/-- Construct a partial order by designating a submonoid with zero support in an abelian group. -/]
abbrev PartialOrder.mkOfSubmonoid : PartialOrder G where
@[to_additive (attr := implicit_reducible)
/-- Construct a partial order by designating a submonoid with zero support in an abelian group. -/]
def PartialOrder.mkOfSubmonoid : PartialOrder G where

is preferred now I believe

{ mul_le_mul_left := fun a b nab c ↦ by simpa [· ≤ ·] using nab }

/-- Construct a linear order by designating
a maximal submonoid with zero support in an abelian group. -/

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
a maximal submonoid with zero support in an abelian group. -/
a maximal submonoid with zero support in an abelian group. -/

Same below

Comment on lines +23 to +26
theorem Subsemiring.nonneg.isPointed [PartialOrder R] [IsOrderedRing R] :
(Subsemiring.nonneg R).IsPointed := AddSubmonoid.nonneg.isPointed R

theorem Subsemiring.nonneg.isSpanning [LinearOrder R] [IsOrderedRing R] :

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
theorem Subsemiring.nonneg.isPointed [PartialOrder R] [IsOrderedRing R] :
(Subsemiring.nonneg R).IsPointed := AddSubmonoid.nonneg.isPointed R
theorem Subsemiring.nonneg.isSpanning [LinearOrder R] [IsOrderedRing R] :
theorem Subsemiring.IsPointed.nonneg [PartialOrder R] [IsOrderedRing R] :
(Subsemiring.nonneg R).IsPointed := AddSubmonoid.nonneg.isPointed R
theorem Subsemiring.IsPointed.nonneg [LinearOrder R] [IsOrderedRing R] :

@YaelDillies YaelDillies left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't really mind that there was no discussion on Zulip. All I care about is that the PR description AND the code document why the situation is different to all the subobjects, so that your refactor doesn't get undone in the future.

@YaelDillies YaelDillies added the awaiting-author A reviewer has asked the author a question or requested changes. label Jun 1, 2026
@mathlib-merge-conflicts mathlib-merge-conflicts 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 Jun 18, 2026
@mathlib-merge-conflicts

Copy link
Copy Markdown

This pull request has conflicts, please merge master and resolve them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting-author A reviewer has asked the author a question or requested changes. large-import Automatically added label for PRs with a significant increase in transitive imports merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) t-algebra Algebra (groups, rings, fields, etc)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants