Skip to content

refactor: make the algebra hierarchy use flat structures#3171

Closed
eric-wieser wants to merge 28 commits intomasterfrom
eric-wieser/flat-algebra
Closed

refactor: make the algebra hierarchy use flat structures#3171
eric-wieser wants to merge 28 commits intomasterfrom
eric-wieser/flat-algebra

Conversation

@eric-wieser
Copy link
Copy Markdown
Member

@eric-wieser eric-wieser commented Mar 29, 2023

By introducing a FlatHack base class and using it as the first parent of every algebra typeclass, it causes the typeclasses to all overlap and so we emulate flat structures.

Most of the pain here comes from to_additive not generating any of the ancestor projections properly; this was already a bug present for n + 1 parents, but now the 1 is taken up

This is one workaround for leanprover/lean4#2074: don't let the eta-diamonds appear in the first place.


Open in Gitpod

@eric-wieser eric-wieser added the WIP Work in progress label Mar 29, 2023
@gebner
Copy link
Copy Markdown
Member

gebner commented Mar 29, 2023

👍 I think that's the best workaround to unblock the port for now.

Can you try to add -DsynthInstance.etaExperiment=true to the lakefile to see if this allows us to enable eta?

@eric-wieser
Copy link
Copy Markdown
Member Author

@gebner: I don't know how to make progress on this unfortunately. The error is

./././Mathlib/Algebra/Ring/Defs.lean:219:50: error: ignoring default value for field 'nsmul' defined at 'Semiring'
./././Mathlib/Algebra/Ring/Defs.lean:340:42: error: ignoring default value for field 'nsmul' defined at 'Semiring'
./././Mathlib/Algebra/Ring/Defs.lean:456:46: error: ignoring default value for field 'nsmul' defined at 'Ring'

which is caused by flat-inheriting the same default field twice. I would be happy to tell lean just to ignore the default value silently.

@gebner
Copy link
Copy Markdown
Member

gebner commented Mar 31, 2023

@eric-wieser I've added a commit that works around that issue.

@gebner
Copy link
Copy Markdown
Member

gebner commented Mar 31, 2023

See leanprover/lean4#2178


theorem lt_of_mul_lt_mul_of_nonpos_left (h : c * a < c * b) (hc : c ≤ 0) : b < a :=
lt_of_mul_lt_mul_left (by rwa [neg_mul, neg_mul, neg_lt_neg_iff]) <| neg_nonneg.2 hc
lt_of_mul_lt_mul_left (by rwa [neg_mul, neg_mul, neg_lt_neg_iff]) <| (neg_nonneg (α := α)).2 hc
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.

and can therefore be easily transported along ring isomorphisms.
Additionaly, this is useful when trying to prove that
a particular ring structure extends to a (semi)field. -/
a particular ring structure extends FlatHack, to a (semi)field. -/
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Overzealous replace I think!

@kim-em kim-em added the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label May 8, 2023
@ghost ghost removed the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Jul 26, 2023
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@gebner, the failure is now happening in Qq in this file. Would you mind taking a look?

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.

Fixed. The function has both a LinearOrderedRing and a DivisionRing instance there, and we can't state that they commute because they're not comparable (I think). We really need a LinearOrderedDivisionRing class.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Sorry, I got tricked by "warnings as errors". Now that I've turned that off, I actually get the error in this file.

I don't seem to be in a monad context, so I'm not sure how to use assumeInstancesCommute

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.

There's assumeInstancesCommute' for term mode. But it's the same situation as in the other function. We have a CommSemiring and a Ring instance, and there's no way to make them defeq.

Copy link
Copy Markdown
Member Author

@eric-wieser eric-wieser Jul 28, 2023

Choose a reason for hiding this comment

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

Presumably we can just override Qq's safety feature here and say "trust me, they're defeq"?

I assume that's what you did in the previous patch; I was struggling to get =Q to work here, but I didn't know about assumeInstancesCommute

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.

The thing is that there is no way to communicate to the lean elaborator that CommSemiring.toSemiring is defeq to Ring.toSemiring. (and that's what's failing here)

If we had for example a CommRing instance i and a CommSemiring instance j, then we could say that the j is defeq to CommRing.toCommSemiring by making j a let-binding. (Which is what =Q does.). In other words the =Q can only make comparable instances commute.

The only workaround I know here is to restate the hypothesis again with the right type.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I've pushed what seems to be a workaround, though now lean is giving me a warning saying that my fix doesn't do anything!

@ghost ghost added the merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) label Aug 1, 2023
@bors bors bot changed the base branch from master to ScottCarnahan/BinomialRing2 September 17, 2023 03:23
@kim-em kim-em changed the base branch from ScottCarnahan/BinomialRing2 to master September 17, 2023 12:18
@harahu
Copy link
Copy Markdown
Contributor

harahu commented Jan 30, 2025

Is this PR abandoned? I'm noticing it hasn't been touched for quite some time. One might as well close it if that is the case.

@grunweg grunweg added the t-algebra Algebra (groups, rings, fields, etc) label Aug 4, 2025
@YaelDillies
Copy link
Copy Markdown
Contributor

Since leanprover/lean4#2074 is now resolved and flat structures are way out of the question now, I believe we can close this?

Feel free to reopen if you disagree.

@YaelDillies YaelDillies deleted the eric-wieser/flat-algebra branch October 20, 2025 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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) WIP Work in progress

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants