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] - fix(algebra/group/units): splitting out mul_one_class for group of units #14923

Closed
wants to merge 15 commits into from

Conversation

joneugster
Copy link
Collaborator

@joneugster joneugster commented Jun 24, 2022

Without this proposed change, the following example gives a (deterministic) timeout:

import algebra.ring.basic

example (R : Type*) [comm_ring R] (a b : Rˣ) : a * (b / a) = b :=
begin
  rw mul_div_cancel'_right,
  -- or: `simp`
end

Note: After a lot of iterations this is now at a short solution that does not change any instance priorities, thus should hopefully not cause future problems. However, I don't understand 100% why splitting out the mul_one_class from the group αˣ instance improves things.

Open in Gitpod

Zulip

@joneugster joneugster added the awaiting-CI The author would like to see what CI has to say before doing more work. label Jun 24, 2022
@joneugster joneugster added easy < 20s of review time. See the lifecycle page for guidelines. awaiting-review The author would like community review of the PR labels Jun 25, 2022
@joneugster joneugster changed the title fix(algebra/group/defs): Instance priority for comm_monoid.to_monoid fix(algebra/group/units): Lower instance priority for units.group Jun 25, 2022
@github-actions github-actions bot removed the awaiting-CI The author would like to see what CI has to say before doing more work. label Jun 25, 2022
@urkud
Copy link
Member

urkud commented Jun 27, 2022

(there was some nonsense here)

@urkud urkud removed the easy < 20s of review time. See the lifecycle page for guidelines. label Jun 27, 2022
@joneugster joneugster added duplicate not-ready-to-merge awaiting-CI The author would like to see what CI has to say before doing more work. and removed awaiting-review The author would like community review of the PR labels Jun 28, 2022
@github-actions github-actions bot removed the awaiting-CI The author would like to see what CI has to say before doing more work. label Jun 29, 2022
@joneugster joneugster added awaiting-review The author would like community review of the PR and removed duplicate not-ready-to-merge labels Jun 29, 2022
@joneugster joneugster changed the title fix(algebra/group/units): Lower instance priority for units.group fix(algebra/group/units): Timeout simplifying division of units in a commutative ring. Jun 29, 2022
@joneugster joneugster changed the title fix(algebra/group/units): Timeout simplifying division of units in a commutative ring. fix(algebra/group/units): Timeout working with division of units in a commutative ring. Jun 29, 2022
@joneugster joneugster changed the title fix(algebra/group/units): Timeout working with division of units in a commutative ring. fix(algebra/group/units): Splitting out mul_one_class for group of units Jul 2, 2022
@joneugster joneugster added the awaiting-CI The author would like to see what CI has to say before doing more work. label Jul 2, 2022
@joneugster joneugster changed the title fix(algebra/group/units): Splitting out mul_one_class for group of units fix(algebra/group/units): splitting out mul_one_class for group of units Jul 2, 2022
@github-actions github-actions bot removed the awaiting-CI The author would like to see what CI has to say before doing more work. label Jul 3, 2022
Copy link
Collaborator

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

However, I don't understand 100% why splitting out the mul_one_class from the group αˣ instance improves things.

It doesn't surprise me that adding an intermediate instance helps things along, since Lean is very slow at checking equality of instances (like units.group) of types (like units) that appear in instances (like units.mul_one_class).

Could you add the timeout example as a test case, for example in a new file test/units.lean?

mul_assoc := λ u₁ u₂ u₃, ext $ mul_assoc u₁ u₂ u₃,
@[to_additive "Additive units of an additive monoid form an additive group."]
instance : group αˣ :=
{ mul_assoc := λ u₁ u₂ u₃, ext $ mul_assoc u₁ u₂ u₃,
Copy link
Collaborator

Choose a reason for hiding this comment

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

We typically copy over data like this, for better unfolding. Or does that reintroduce the timeout?

Suggested change
{ mul_assoc := λ u₁ u₂ u₃, ext $ mul_assoc u₁ u₂ u₃,
{ mul := (*),
one := 1,
mul_assoc := λ u₁ u₂ u₃, ext $ mul_assoc u₁ u₂ u₃,

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok, made your suggested changes and added the test case, thx.

Out of curiosity, why does this improve unfolding if you specify mul and one like this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

IIRC, specifying mul means the unfolded version of units.group will give theorems stated using * instead of mul_one_class.to_has_mul.mul or something like that.

@Vierkantor Vierkantor added awaiting-author A reviewer has asked the author a question or requested changes and removed awaiting-review The author would like community review of the PR labels Jul 12, 2022
Copy link
Member

@eric-wieser eric-wieser left a comment

Choose a reason for hiding this comment

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

I'm much happier with this than the previous priority-based solutions. Just a small comment, and Anne's suggestion to go.

@joneugster joneugster added awaiting-review The author would like community review of the PR and removed awaiting-author A reviewer has asked the author a question or requested changes labels Jul 13, 2022
Copy link
Collaborator

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

Thanks!

bors r+

@leanprover-community-bot-assistant leanprover-community-bot-assistant added ready-to-merge All that is left is for bors to build and merge this PR. (Remember you need to say `bors r+`.) and removed awaiting-review The author would like community review of the PR labels Jul 13, 2022
bors bot pushed a commit that referenced this pull request Jul 13, 2022
…units (#14923)


Without this proposed change, the following example gives a `(deterministic) timeout`:
```lean
import algebra.ring.basic

example (R : Type*) [comm_ring R] (a b : Rˣ) : a * (b / a) = b :=
begin
  rw mul_div_cancel'_right,
  -- or: `simp`
end
```


Co-authored-by: Jon Eugster <j.eugster@sms.ed.ac.uk>
@bors
Copy link

bors bot commented Jul 13, 2022

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title fix(algebra/group/units): splitting out mul_one_class for group of units [Merged by Bors] - fix(algebra/group/units): splitting out mul_one_class for group of units Jul 13, 2022
@bors bors bot closed this Jul 13, 2022
@bors bors bot deleted the je_comm_ring_to_comm_monoid branch July 13, 2022 16:39
joelriou pushed a commit that referenced this pull request Jul 23, 2022
…units (#14923)


Without this proposed change, the following example gives a `(deterministic) timeout`:
```lean
import algebra.ring.basic

example (R : Type*) [comm_ring R] (a b : Rˣ) : a * (b / a) = b :=
begin
  rw mul_div_cancel'_right,
  -- or: `simp`
end
```


Co-authored-by: Jon Eugster <j.eugster@sms.ed.ac.uk>
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

5 participants