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

Generalize exp_ineq1 and add exp_ineq1_le, which holds forall Reals. #13582

Merged
merged 1 commit into from
Dec 11, 2020

Conversation

shinnar
Copy link
Contributor

@shinnar shinnar commented Dec 6, 2020

Adds exp_ineq2, the negative counterpart to exp_ineq1.
Adds exp_ineq, which holds for all reals (but is a <= instead of a <).

Rpower currently defines exp_ineq1 : forall x:R, 0 < x -> 1 + x < exp x.
This PR adds the corresponding lemma for negative R: exp_ineq2 : forall x:R, 0 > x -> 1 + x < exp x
and the (slightly weaker) version for all R: exp_ineq : forall x:R, 1 + x <= exp x
These minor additions are nice to add for completeness, and are useful for users.

This proof was mostly done by Barry Trager (@bmtrager), and is being submitted with his permission (and at his request).

Kind: documentation / bug fix / feature / performance / infrastructure.
library addition / minor feature

Fixes / closes #????

  • Added / updated test-suite

@shinnar shinnar requested a review from a team as a code owner December 6, 2020 03:07
@jashug jashug added the part: standard library The standard library stdlib. label Dec 6, 2020
@Zimmi48 Zimmi48 added kind: feature New user-facing feature request or implementation. needs: changelog entry This should be documented in doc/changelog. labels Dec 6, 2020
@Zimmi48 Zimmi48 added this to the 8.14+beta1 milestone Dec 6, 2020
@Zimmi48
Copy link
Member

Zimmi48 commented Dec 6, 2020

This proof was mostly done by Barry Trager (@bmtrager), and is being submitted with his permission (and at his request).

Note that you can use Co-authored-by: to credit them. See https://github.blog/2018-01-29-commit-together-with-co-authors/

Also, this addition to the standard library, as any addition, will need a changelog entry. See https://github.com/coq/coq/tree/master/doc/changelog#how-to-add-an-entry

theories/Reals/Rpower.v Outdated Show resolved Hide resolved
@retzger77
Copy link

retzger77 commented Dec 6, 2020 via email

@shinnar
Copy link
Contributor Author

shinnar commented Dec 6, 2020

should I update the PR as suggested above? Note that I believe it would be a (minorly) breaking change (hence the original proposal).

@thery
Copy link
Contributor

thery commented Dec 7, 2020

exp_ineq1 should not be much used, so generalize it to the whole line except zero should be ok.

@shinnar shinnar force-pushed the exp_ineq branch 2 times, most recently from 1ffa4f9 to 67a065b Compare December 7, 2020 03:19
@Zimmi48
Copy link
Member

Zimmi48 commented Dec 7, 2020

It turns out that there is one external project among the ones we test in CI that was using this lemma (https://gitlab.inria.fr/coquelicot/coquelicot/-/blob/master/theories/ElemFct.v#L604). Since his maintainer is also a maintainer of the real library, let's ping him here (@silene).

What are the paths to making a backward-compatible fix for this project possible? A #[ deprecated(since="8.14", note="Use exp_ineq1_pos instead.") ] Notation exp_ineq1 := exp_ineq1_pos.?

Note that in the current state of this PR (as opposed to the initial state), the type of changelog entry should be Changed instead of Added.

@shinnar shinnar force-pushed the exp_ineq branch 2 times, most recently from 9ee9784 to 646759d Compare December 7, 2020 13:17
@thery
Copy link
Contributor

thery commented Dec 8, 2020

Looks like @silene could use the new exp_ineq!.
To be backward compatible one possibility is to replace
by apply Rlt_le, exp_ineq1
with
apply Rlt_le, exp_ineq1; lra or
apply Rlt_le, exp_ineq1; auto with real if one doesn't want to import lra.

@thery
Copy link
Contributor

thery commented Dec 8, 2020

Maybe we could get rid of the _neg and _pos theorems that are not so useful and directly prove ineq1

Lemma exp_ineq1 : forall x : R, x <> 0 -> 1 + x < exp x.
Proof.
assert (Hd : forall c : R,
           derivable_pt_lim (fun x : R => exp x - (x + 1)) c (exp c - 1)).
  intros.
  apply derivable_pt_lim_minus; [apply derivable_pt_lim_exp | ].
  replace (1) with (1 + 0) at 1 by lra.
  apply derivable_pt_lim_plus;
    [apply derivable_pt_lim_id | apply derivable_pt_lim_const].
intros x xdz; destruct (Rtotal_order x 0)  as [xlz|[xez|xgz]].
- destruct (MVT_cor2 _ _ x 0 xlz (fun c _ => Hd c)) as [c [HH1 HH2]].
  rewrite exp_0 in HH1.
  assert (H1 : 0 < x * exp c - x); [| lra].
  assert (H2 : x * exp 0  < x * exp c); [| rewrite exp_0 in H2; lra].
  apply Rmult_lt_gt_compat_neg_l; auto.
  now apply exp_increasing.
- now case xdz.
- destruct (MVT_cor2 _ _ 0 x xgz (fun c _ => Hd c)) as [c [HH1 HH2]].
  rewrite exp_0 in HH1.
  assert (H1 : 0 < x * exp c - x); [| lra].
  assert (H2 : x * exp 0  < x * exp c); [| rewrite exp_0 in H2; lra].
  apply Rmult_lt_compat_l; auto.
  now apply exp_increasing.
Qed.

@thery thery changed the title Generalize exp_ineq1 to exp_ineq, which holds forall Reals. Generalize exp_ineq1 and add exp_ineq1_le, which holds forall Reals. Dec 9, 2020
@thery
Copy link
Contributor

thery commented Dec 9, 2020

ok I've submitted the overlay for coquelicot #4. As soon as @silene accepts it I think we can merge
Ps: @shinnar I've modified the proof of exp_ineq1 in my comment I think it looks more symmetrical now

@Zimmi48 Zimmi48 removed the request for review from a team December 9, 2020 10:02
@Zimmi48 Zimmi48 removed the needs: changelog entry This should be documented in doc/changelog. label Dec 9, 2020
@shinnar
Copy link
Contributor Author

shinnar commented Dec 9, 2020

@thery updated. Given that the current form of the proof is yours, should I include some form of attribution in the commit message? If so, what?

@Zimmi48
Copy link
Member

Zimmi48 commented Dec 9, 2020

One way is to put a "with help from" or "with review of Laurent Théry" in the changelog entry.

The original (which holds only for positive numbers) is now called exp_ineq1_pos.
A version that holds only for negative numbers is added as exp_ineq1_neg.
Adds exp_ineq1_le, which holds for all reals (but is a <= instead of a <).

Co-authored-by: Barry M. Trager <bmt@us.ibm.com>
@thery
Copy link
Contributor

thery commented Dec 9, 2020

@shinnar the proof is yours I have just reshaped it.

@shinnar
Copy link
Contributor Author

shinnar commented Dec 9, 2020

@shinnar the proof is yours I have just reshaped it.

@thery Would you like to be included in the changelog? I amended it to read
"by Avi Shinnar and Barry Trager, with help from Laurent Théry", which seems appropriate.
If you would prefer to not be credited, I can revert the change.

@thery
Copy link
Contributor

thery commented Dec 9, 2020

fine with me, thanks for your contribution!

@thery
Copy link
Contributor

thery commented Dec 11, 2020

ok the overlay is in coquelicot #4, so we can merge

@thery
Copy link
Contributor

thery commented Dec 11, 2020

@coqbot: merge now

@coqbot-app
Copy link
Contributor

coqbot-app bot commented Dec 11, 2020

@thery: You can't merge the PR because you're not among the assignees.

@thery thery self-assigned this Dec 11, 2020
@thery
Copy link
Contributor

thery commented Dec 11, 2020

@coqbot: merge now

@coqbot-app coqbot-app bot merged commit d40ef24 into coq:master Dec 11, 2020
@shinnar shinnar deleted the exp_ineq branch December 11, 2020 13:38

(`#13582 <https://github.com/coq/coq/pull/13582>`_,
by Avi Shinnar and Barry Trager, with help from Laurent Théry

Copy link
Contributor

@olaure01 olaure01 Dec 28, 2020

Choose a reason for hiding this comment

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

The white line an final parenthesis seem to generate errors when adding new changelogs after this one (see for example CI errors in #13671 ).
If I am right and since this is already merged, what is the best way to make a modification? create an adhoc new PR?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, you are right about the diagnosis. You can either open an ad hoc PR or fix it in your own PR, especially if your own PR shouldn't take too long to merge.

olaure01 added a commit to olaure01/coq that referenced this pull request Dec 28, 2020
olaure01 added a commit to olaure01/coq that referenced this pull request Mar 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: feature New user-facing feature request or implementation. part: standard library The standard library stdlib.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants