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/monoid_algebra/basic): add int_cast to monoid_algebra instances #15779

Closed
wants to merge 16 commits into from

Conversation

ericrbg
Copy link
Collaborator

@ericrbg ericrbg commented Jul 31, 2022

Note that this reshuffles how some instances are created to ensure that they get the int_cast field.


Open in Gitpod

@ericrbg ericrbg added awaiting-CI The author would like to see what CI has to say before doing more work. awaiting-review The author would like community review of the PR labels Jul 31, 2022
@ericrbg ericrbg requested a review from eric-wieser July 31, 2022 17:56
@mathlib-dependent-issues-bot mathlib-dependent-issues-bot added the blocked-by-other-PR This PR depends on another PR which is still in the queue. A bot manages this label via PR comment. label Jul 31, 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.

LGTM, although the timeout is worrying. Let's wait for the PR this depends on before moving forward.

@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 31, 2022
ericrbg and others added 2 commits August 1, 2022 10:32
Co-authored-by: Anne Baanen <Vierkantor@users.noreply.github.com>
Co-authored-by: Eric Wieser <wieser.eric@gmail.com>
Co-authored-by: Eric Wieser <wieser.eric@gmail.com>
@mathlib-dependent-issues-bot mathlib-dependent-issues-bot removed the blocked-by-other-PR This PR depends on another PR which is still in the queue. A bot manages this label via PR comment. label Aug 1, 2022
@mathlib-dependent-issues-bot
Copy link
Collaborator

@@of_irreducible_map _ _ _ (is_local_ring_hom_expand R hp.bot_lt) hf
let _ := is_local_ring_hom_expand R hp.bot_lt in by exactI of_irreducible_map ↑(expand R p) hf
Copy link
Member

Choose a reason for hiding this comment

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

Any idea what happened here?

Copy link
Collaborator Author

@ericrbg ericrbg Aug 1, 2022

Choose a reason for hiding this comment

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

not really sure, but the typechecking has been crazy brittle with int_cast and stuff: for some further examples that bewilder me, look at this and its fix [and no, deinstancing the comm_semiring instance didn't help at all]

e: the error isn't obvious, it's https://github.com/leanprover-community/mathlib/runs/7601089733?check_suite_focus=true and the only way I managed to fix it without just undoing my changes was by using refine like 5 times to define that function (and the equivalent version for mv_polynomial further down the file)

Copy link
Member

Choose a reason for hiding this comment

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

Either way, I guess this is nicer because it avoids an @

@@ -173,6 +173,9 @@ rfl
/-- `polynomial.algebra_of_algebra` is consistent with `algebra_nat`. -/
example [comm_semiring R] : (polynomial.algebra_of_algebra : algebra ℕ R[X]) = algebra_nat := rfl

/-- `polynomial.algebra_of_algebra` is consistent with `algebra_int`. -/
example [ring R] : (polynomial.algebra_of_algebra : algebra ℤ R[X]) = algebra_int _ := rfl
Copy link
Member

Choose a reason for hiding this comment

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

I guess the line above only needs semiring R then, my mistake.

test/instance_diamonds.lean Outdated Show resolved Hide resolved
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.

bors d+

@bors
Copy link

bors bot commented Aug 1, 2022

✌️ ericrbg 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-bot-assistant leanprover-community-bot-assistant added delegated The PR author may merge after reviewing final suggestions. and removed awaiting-review The author would like community review of the PR labels Aug 1, 2022
@ericrbg
Copy link
Collaborator Author

ericrbg commented Aug 1, 2022

bors r+

bors bot pushed a commit that referenced this pull request Aug 1, 2022
… instances (#15779)

Note that this reshuffles how some instances are created to ensure that they get the `int_cast` field.



Co-authored-by: Eric Rodriguez <37984851+ericrbg@users.noreply.github.com>
@bors
Copy link

bors bot commented Aug 1, 2022

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title fix(algebra/monoid_algebra/basic): add int_cast to monoid_algebra instances [Merged by Bors] - fix(algebra/monoid_algebra/basic): add int_cast to monoid_algebra instances Aug 1, 2022
@bors bors bot closed this Aug 1, 2022
@bors bors bot deleted the ericrbg/poly_diamond_int branch August 1, 2022 18:00
robertylewis pushed a commit that referenced this pull request Aug 2, 2022
… instances (#15779)

Note that this reshuffles how some instances are created to ensure that they get the `int_cast` field.



Co-authored-by: Eric Rodriguez <37984851+ericrbg@users.noreply.github.com>
khwilson pushed a commit that referenced this pull request Aug 2, 2022
… instances (#15779)

Note that this reshuffles how some instances are created to ensure that they get the `int_cast` field.



Co-authored-by: Eric Rodriguez <37984851+ericrbg@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
delegated The PR author may merge after reviewing final suggestions.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants