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/ordered*): add norm_cast attribute #3132

Closed
wants to merge 7 commits into from

Conversation

kbuzzard
Copy link
Member


A few with_top and with_bot coercion lemmas didn't have the norm_cast attribute. I added them.

Comments:

  1. I went to the docs to read about which lemmas should be tagged, but the recent refactor which meant that everything should be tagged norm_cast also entailed a cleanup of the docs, so I was a bit in the dark as to exactly what I should be tagging. I went for an "if it moves, norm_cast it" approach on the basis that it would be easier for a reviewer to say "no, you don't tag that kind of lemma" than for a reviewer to say "you missed this one, 100 lines up".

  2. The norm_cast attribute was not available in order.bounded_lattice so I just add the attribute to the coe lemmas from that file in the ordered_group file, where the attribute was available. This can't be the right answer. But again I took the approach that it was better to do the wrong thing than just ignore the problem completely and then forget about it.

  3. coe_zero and coe_eq_zero were only written for canonically ordered rings or something -- I've changed it so that they apply to additive monoids. Why can't they just apply to with_zero? Is there a problem with defining a zero on with_top X to be the zero of X, whenever X has a zero?

Copy link
Member

@robertylewis robertylewis left a comment

Choose a reason for hiding this comment

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

These all look reasonable! Just suspicious of the two I've tagged, and any others that norm_cast can already prove. It's not necessarily a problem to tag them anyway, just not needed.

@[simp, norm_cast] lemma coe_zero {α : Type*}
[add_monoid α] : ((0 : α) : with_top α) = 0 := rfl

@[simp, norm_cast] lemma coe_eq_zero {α : Type*}
Copy link
Member

Choose a reason for hiding this comment

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

If this lemma is proved by norm_cast it probably shouldn't need to be tagged norm_cast itself. Does something fail without this? what norm_cast lemmas does it use? Are other lemmas in the file provable with norm_cast already?

Copy link
Member Author

@kbuzzard kbuzzard Jun 22, 2020

Choose a reason for hiding this comment

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

Answers: 1) Nothing seems to fail if I untag it 2) The only way I know how to answer this is with set_option trace.simplify.rewrite true, which tells me coe_eq_zero is proved using coe_zero. There must be more to it than this though; norm_cast is doing something simp isn't doing. 3) with_bot.coe_zero can be proved by norm_cast, using with_top.coe_zero (see below)

@[simp] lemma coe_zero [add_monoid α] : ((0 : α) : with_bot α) = 0 := rfl
@[norm_cast] lemma coe_zero [add_monoid α] : ((0 : α) : with_bot α) = 0 := rfl

@[norm_cast] lemma coe_eq_zero {α : Type*}
Copy link
Member

Choose a reason for hiding this comment

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

As above, the proof is already norm_cast.

Copy link
Member Author

Choose a reason for hiding this comment

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

In fact norm_cast can even prove coe_zero -- it uses with_top proofs to prove with_bot theorems, presumably because the with_bot instances are defined using with_top instances rather than being redefined. I don't know if this is good or bad.

@gebner
Copy link
Member

gebner commented Jun 22, 2020

You probably want coe_bit0 and coe_bit1 as well, otherwise norm_cast doesn't work on numerals. (I hope I didn't miss them.)

@robertylewis robertylewis added the awaiting-author A reviewer has asked the author a question or requested changes label Jun 22, 2020
@kbuzzard
Copy link
Member Author

I added coe_bit0 and coe_bit1 for both with_bot and with_top. I am a bit confused about whether we even need these lemmas for with_bot. I don't know if they're good simp lemmas, and norm_cast can prove the with_bot lemmas using the with_top lemmas. I added them on the basis that the library should be complete, but I didn't tag the with anything. I tagged with_top.coe_add with simp and it broke the proof of add_eq_top [ordered_add_comm_monoid α] : a + b = ⊤ ↔ a = ⊤ ∨ b = ⊤. I figured who was to say which was simpler out of \u(a+b) and \u a + \u b -- sometimes you want to push coercions in and sometimes pull them out -- there's no obvious "go this way" route.

@kbuzzard kbuzzard added awaiting-review The author would like community review of the PR WIP Work in progress and removed awaiting-author A reviewer has asked the author a question or requested changes awaiting-review The author would like community review of the PR labels Jun 22, 2020
@kbuzzard kbuzzard added WIP Work in progress and removed WIP Work in progress labels Jun 22, 2020
@kbuzzard kbuzzard added awaiting-review The author would like community review of the PR and removed WIP Work in progress labels Jun 22, 2020
@robertylewis
Copy link
Member

I'm afraid I was wrong when I wrote

If this lemma is proved by norm_cast it probably shouldn't need to be tagged norm_cast itself

because of norm_cast's special handling for numerals. I've reverted a few of the attributes you deleted, because I think they're actually not redundant. (I can construct examples that work only with the attribute.) The others, AFAICT, aren't needed.

There shouldn't be an issue importing norm_cast earlier, so I moved some of the attributions to bounded_lattice where they belong.

@robertylewis
Copy link
Member

bors merge

@github-actions github-actions bot 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 Jun 23, 2020
bors bot pushed a commit that referenced this pull request Jun 23, 2020
Co-authored-by: Rob Lewis <rob.y.lewis@gmail.com>
@bors
Copy link

bors bot commented Jun 23, 2020

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title fix(algebra/ordered*): add norm_cast attribute [Merged by Bors] - fix(algebra/ordered*): add norm_cast attribute Jun 23, 2020
@bors bors bot closed this Jun 23, 2020
@bors bors bot deleted the withtop-normcast branch June 23, 2020 16:03
cipher1024 pushed a commit to cipher1024/mathlib that referenced this pull request Mar 15, 2022
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

3 participants