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

Don't call Maxima with no-variable symbolic relation tests #24658

Closed
rwst opened this issue Feb 5, 2018 · 40 comments
Closed

Don't call Maxima with no-variable symbolic relation tests #24658

rwst opened this issue Feb 5, 2018 · 40 comments

Comments

@rwst
Copy link

rwst commented Feb 5, 2018

In the case a relation without variables is to be decided, the procedure is to use Pynac, then RIF and, if it cannot be decided, Maxima. This is fine with expressions containing variables as Maxima can do some proofs. Expressions without variables are handled poorly as the example shows:

sage: val = pi - 2286635172367940241408/1029347477390786609545*sqrt(2)
sage: bool(val>0)
True

(%i2) is (%pi-(1116521080257783321*2^(23/2))/1029347477390786609545>0);
(%o2)                                true

This ticket changes the procedure to no longer try the unreliable Maxima after RIF has failed in a relation without variables. It will just return False.

CC: @simon-king-jena

Component: symbolics

Author: Ralf Stephan

Branch/Commit: e78e84b

Reviewer: Simon King, Matthias Koeppe

Issue created by migration from https://trac.sagemath.org/ticket/24658

@rwst rwst added this to the sage-8.2 milestone Feb 5, 2018
@rwst
Copy link
Author

rwst commented Feb 5, 2018

@rwst
Copy link
Author

rwst commented Feb 5, 2018

New commits:

0a29d4024658: Don't call Maxima with no-variable symbolic relation tests

@rwst

This comment has been minimized.

@rwst
Copy link
Author

rwst commented Feb 5, 2018

Commit: 0a29d40

@rwst
Copy link
Author

rwst commented Feb 5, 2018

Author: Ralf Stephan

@simon-king-jena
Copy link
Member

comment:5

It should be easy to review (after all, it is just working around a short-coming of maxima), and to me it looks good. However, I do not feel particularly confident about symbolic expressions (which I normally try to avoid by all means).

Also, I wonder about that change:

diff --git a/src/sage/symbolic/expression.pyx b/src/sage/symbolic/expression.pyx
index 7130898..0f943d9 100644
--- a/src/sage/symbolic/expression.pyx
+++ b/src/sage/symbolic/expression.pyx
@@ -2612,7 +2612,7 @@ cdef class Expression(CommutativeRingElement):
         The :meth:`~sage.structure.element.Element.is_zero` method
         is more capable::
 
-            sage: t = pi + (pi - 1)*pi - pi^2
+            sage: t = pi + x*pi + (pi - 1 - x)*pi - pi^2
             sage: t.is_trivial_zero()
             False
             sage: t.is_zero()

Why did you change it? Apparently in order to have a variable in it. So, without the variable, t.is_trivial_zero() would return True, right? Which somehow makes sense, actually. I believe one should retain both tests, to show-case that Sage can now detect some more expressions which are trivially zero.

@simon-king-jena
Copy link
Member

comment:6

PS: Also I think that in the new test val = pi - 2286635172367940241408/1029347477390786609545*sqrt(2) (which actually comes out of Ramanujan's formula for pi) the ticket number should be mentioned.

@rwst
Copy link
Author

rwst commented Feb 5, 2018

comment:7

Replying to @simon-king-jena:

Also, I wonder about that change:

diff --git a/src/sage/symbolic/expression.pyx b/src/sage/symbolic/expression.pyx
index 7130898..0f943d9 100644
--- a/src/sage/symbolic/expression.pyx
+++ b/src/sage/symbolic/expression.pyx
@@ -2612,7 +2612,7 @@ cdef class Expression(CommutativeRingElement):
         The :meth:`~sage.structure.element.Element.is_zero` method
         is more capable::
 
-            sage: t = pi + (pi - 1)*pi - pi^2
+            sage: t = pi + x*pi + (pi - 1 - x)*pi - pi^2
             sage: t.is_trivial_zero()
             False
             sage: t.is_zero()

Why did you change it?

Because getting what we want (avoid Maxima numeric) and keeping the doctest would need more involved code, basically checking if there are constants, if so substituting them for variables, and then calling Maxima with that. Note that the changed result is not wrong so I'll add it again.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 5, 2018

Changed commit from 0a29d40 to 17827ea

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 5, 2018

Branch pushed to git repo; I updated commit sha1. New commits:

17827ea24658: address reviewer's comments

@rwst
Copy link
Author

rwst commented Feb 5, 2018

comment:9

Replying to @simon-king-jena:

So, without the variable, t.is_trivial_zero() would return True, right? Which somehow makes sense, actually. I believe one should retain both tests, to show-case that Sage can now detect some more expressions which are trivially zero.

No is_trivial_zero test for trivial zero, i.e., 0.

@simon-king-jena
Copy link
Member

comment:10

Hm. (pi + (pi - 1)*pi - pi^2).is_zero() returning False goes to far, IMHO.

If I understand correctly, the underlying bug is in Maxima's test for positivity/negativity. Is there a corresponding bug in Maxima's test for being zero as well? If it isn't, then I guess one should return to using Maxima for is_zero.

@simon-king-jena
Copy link
Member

comment:11

Or, alternatively, is_zero() (but not is_trivial_zero()) should be cleverer in using RIF:

sage: RIF(pi + (pi - 1)*pi - pi^2)>0
False
sage: RIF(pi + (pi - 1)*pi - pi^2)<0
False
sage: RIF(pi + (pi - 1)*pi - pi^2)==0
False
sage: RIF(pi + (pi - 1)*pi - pi^2).is_NaN()
False

Thus, we have a real number (it is not !NaN) that is neither positive nor negative, and thus it would make sense to treat it as zero, although RIF is careful enough to not assert that it actually is zero.

Or prepend it by a simplification?

sage: RIF((pi + (pi - 1)*pi - pi^2).simplify_full()).is_zero()
True

After all, presumably is_zero() is supposed to invest more work into testing than is_trivial_zero().

@rwst
Copy link
Author

rwst commented Feb 5, 2018

comment:12

Replying to @simon-king-jena:

Thus, we have a real number (it is not !NaN) that is neither positive nor negative, and thus it would make sense to treat it as zero, although RIF is careful enough to not assert that it actually is zero.

Your own example (val) above would then be treated as zero too.

Or prepend it by a simplification?

That is probably best. One can always construct cases that need long time like ((pi+1)^10000 - pi*(pi+1)^9999 - (pi+1)^9999).simplify_full().

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 5, 2018

Changed commit from 17827ea to 590600b

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 5, 2018

Branch pushed to git repo; I updated commit sha1. New commits:

590600b24658: if test_relation fails try again simplified

@rwst
Copy link
Author

rwst commented Feb 5, 2018

comment:14

OK, this looks fine in symbolics, there may be failing doctests elsewhere.

@rwst
Copy link
Author

rwst commented Feb 5, 2018

comment:15

There is a doctest in manifolds that relies on -arctanh(1/2) + 1/2*log(3) == 0. That should be resolved by implementing more special values for atanh. Surprise, I thought we had them all.

@rwst
Copy link
Author

rwst commented Feb 5, 2018

comment:16

Maxima cannot simplify arctanh(1/2) - 1/2*log(3). The question is do we formulate the doctest differently, or do we implement rewrite of inverse hyperbolic expressions, e.g. for integer and rational number arguments, either immediately or inside simplify_full.

@rwst
Copy link
Author

rwst commented Feb 6, 2018

Dependencies: #24668

@rwst
Copy link
Author

rwst commented Feb 6, 2018

comment:17

The new Pynac has the atanh simpifications and so we depend on that upgrade to fix the manifolds doctest fail.

@rwst
Copy link
Author

rwst commented Feb 6, 2018

comment:18

This one is serious because it does happen only when doctesting, and bool(SR(...)) does not seem to be involved directly.

File "src/sage/structure/parent.pyx", line 1514, in sage.structure.parent.Parent.hom._is_coercion_cached
Failed example:
    R._is_coercion_cached(QQ)
Expected:
    False
Got:
    True

It goes away if I replace QQ with QQbar. I think that the coercion cache is not really erased with creation of a new parent, and the additional call to test_relation in this ticket fills it with something. There are doctest calls to bool(SR(...)) that happen before that doctest in parent.pyx, let's see...

@rwst
Copy link
Author

rwst commented Feb 6, 2018

comment:19

Got it, it comes from the doctest sqrt(2) in CC in line 1051 of pynac.pyx. How can I turn off that it influences the doctest in line 1514?

@rwst
Copy link
Author

rwst commented Feb 6, 2018

comment:20

Replying to @rwst:

Got it, it comes from the doctest sqrt(2) in CC in line 1051 of parent.pyx. How can I turn off that it influences the doctest in line 1514?

@rwst
Copy link
Author

rwst commented Feb 6, 2018

comment:21

I meant to write parent.pyx.

@rwst
Copy link
Author

rwst commented Feb 7, 2018

Changed commit from 590600b to e78e84b

@rwst
Copy link
Author

rwst commented Feb 19, 2018

comment:27

The manifolds test passes now so we trigger the sleeping patchbots.

@fchapoton
Copy link
Contributor

Changed dependencies from #24668 #24673 to none

@fchapoton fchapoton modified the milestones: sage-8.2, sage-8.7 Feb 24, 2019
@embray
Copy link
Contributor

embray commented Mar 25, 2019

comment:30

Ticket retargeted after milestone closed (if you don't believe this ticket is appropriate for the Sage 8.8 release please retarget manually)

@embray embray modified the milestones: sage-8.7, sage-8.8 Mar 25, 2019
@embray
Copy link
Contributor

embray commented Jul 3, 2019

comment:31

Moving tickets from the Sage 8.8 milestone that have been actively worked on in the last six months to the next release milestone (optimistically).

@embray embray modified the milestones: sage-8.8, sage-8.9 Jul 3, 2019
@embray
Copy link
Contributor

embray commented Dec 30, 2019

comment:32

Ticket retargeted after milestone closed

@embray embray modified the milestones: sage-8.9, sage-9.1 Dec 30, 2019
@mkoeppe
Copy link
Contributor

mkoeppe commented May 1, 2020

comment:33

Moving tickets to milestone sage-9.2 based on a review of last modification date, branch status, and severity.

@mkoeppe mkoeppe modified the milestones: sage-9.1, sage-9.2 May 1, 2020
@mkoeppe
Copy link
Contributor

mkoeppe commented Aug 15, 2020

Reviewer: Simon King, Matthias Koeppe

@mkoeppe
Copy link
Contributor

mkoeppe commented Aug 15, 2020

comment:34

This is a clear improvement.

@vbraun
Copy link
Member

vbraun commented Aug 20, 2020

Changed branch from u/rws/24658 to e78e84b

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants