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

Add reg theorems for nat on multiplication #17560

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

HaoYang670
Copy link
Contributor

Fixes / closes #17354

Signed-off-by: remzi <13716567376yh@gmail.com>
@HaoYang670 HaoYang670 requested a review from a team as a code owner May 2, 2023 13:38
@coqbot-app coqbot-app bot added the needs: full CI The latest GitLab pipeline that ran was a light CI. Say "@coqbot run full ci" to get a full CI. label May 2, 2023
@proux01
Copy link
Contributor

proux01 commented May 2, 2023

We already have

Lemma eqn_pmul2l m n1 n2 : 0 < m -> (m * n1 == m * n2) = (n1 == n2).
Lemma eqn_pmul2r m n1 n2 : 0 < m -> (n1 * m == n2 * m) = (n1 == n2).

why would you need a weaker version? (if the implication is really needed, one can use eqbLR or eqbRL)

@HaoYang670
Copy link
Contributor Author

Hi @robbertkrebbers, do we still need these?

@proux01
Copy link
Contributor

proux01 commented May 3, 2023

Ho, sorry it's about Coq stdlib here. I accidentally mixed up with MathComp, so please disregard my comment above.
I have no opinion about that part of the the stdlib since I'm not using it.

@robbertkrebbers
Copy link
Contributor

Thanks for the PR.

I think it's important that Z and other number types are consistent. So we should either have this lemma for all Numbers instances, or none at all. Otherwise it's confusing.

If we want to have them for all, we should remove the specific versions for Z and the comment, see https://github.com/coq/coq/blob/master/theories/ZArith/BinInt.v#L1281

If we want to have them for none at all, we should deprecated the versions for Z and have a warning that says to use the <-> lemma instead.

@HaoYang670
Copy link
Contributor Author

Hi @robbertkrebbers, should we define a type class for all number types?

@robbertkrebbers
Copy link
Contributor

There is a module in Coq for all number types, and my suggestion is that the reg lemmas should be part of that module.

Signed-off-by: remzi <13716567376yh@gmail.com>
@HaoYang670
Copy link
Contributor Author

Hi @robbertkrebbers, could I know which module it is?

@robbertkrebbers
Copy link
Contributor

NZMulOrderProp I think, here's the abstract version of the biimplication that was mentioned in the issue: https://github.com/coq/coq/blob/master/theories/Numbers/NatInt/NZMulOrder.v#L100

Signed-off-by: remzi <13716567376yh@gmail.com>
Signed-off-by: remzi <13716567376yh@gmail.com>
@HaoYang670
Copy link
Contributor Author

Move mul_reg_l and mul_reg_r from BigInt to NZMulOrder.

@proux01
Copy link
Contributor

proux01 commented May 10, 2023

Note that if you (re)move lemmas, you need a deprecation (https://coq.inria.fr/distrib/current/refman/using/libraries/writing.html).

@robbertkrebbers
Copy link
Contributor

Note that if you (re)move lemmas, you need a deprecation (https://coq.inria.fr/distrib/current/refman/using/libraries/writing.html).

Unless I am mistaken, you can still refer to the Z lemmas by Z.lem. It merely got moved to a different place and generalized from Z to all instances of Numbers.

@robbertkrebbers
Copy link
Contributor

By the way, it appears there are plenty of more lemmas specific to Z in https://github.com/coq/coq/blob/master/theories/ZArith/BinInt.v#L1281 which should either be generalized to Numbers or be deprecated.

@HaoYang670
Copy link
Contributor Author

Thank you, @proux01 @robbertkrebbers for your review!

Note that if you (re)move lemmas, you need a deprecation (https://coq.inria.fr/distrib/current/refman/using/libraries/writing.html).

Unless I am mistaken, you can still refer to the Z lemmas by Z.lem. It merely got moved to a different place and generalized from Z to all instances of Numbers.

I am not familiar with the module type in Coq. But actually, I can't refer Z.mul_reg_l or Z.mul_reg_r. I need to check if Z implements NZMulOrderProp.

By the way, it appears there are plenty of more lemmas specific to Z in https://github.com/coq/coq/blob/master/theories/ZArith/BinInt.v#L1281 which should either be generalized to Numbers or be deprecated.

Good point!. I filed #17606 to track it

@HaoYang670
Copy link
Contributor Author

@coqbot run full ci

@Zimmi48
Copy link
Member

Zimmi48 commented May 30, 2023

Sorry, @HaoYang670, this coqbot command only works if you're a member of @coq/contributors. We should make coqbot provide feedback when it ignores it. Let me re-run it for you.

@coqbot run full CI

@coqbot-app coqbot-app bot removed the needs: full CI The latest GitLab pipeline that ran was a light CI. Say "@coqbot run full ci" to get a full CI. label May 30, 2023
@ppedrot ppedrot added the part: standard library The standard library stdlib. label Oct 30, 2023
@SkySkimmer
Copy link
Contributor

Not sure if CI failures are real, at least some of them are not so let's get a fresh run
@coqbot run full ci

@coq/stdlib-maintainers please take a decision on this PR instead of waiting another year

@andres-erbsen
Copy link
Contributor

Let's merge if CI passes.

@andres-erbsen andres-erbsen self-assigned this Apr 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
part: standard library The standard library stdlib.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cancelation lemmas for multiplication on nat are missing
7 participants