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

[rws]

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.

[rws]

New commits:

​0a29d40

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

[SimonKing]

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:

src/sage/symbolic/expression.pyx

@@ -2612 +2612 @@
         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.

[SimonKing]

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.

[rws]

Replying to SimonKing:

Also, I wonder about that change:

src/sage/symbolic/expression.pyx

@@ -2612 +2612 @@
         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.

[git]

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

​17827ea

24658: address reviewer's comments

[rws]

Replying to SimonKing:

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.

[SimonKing]

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.

[SimonKing]

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().

[rws]

Replying to SimonKing:

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().

[git]

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

​590600b

24658: if test_relation fails try again simplified

[rws]

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

[rws]

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.

[rws]

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.

[rws]

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

[rws]

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...

[rws]

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?

[rws]

Replying to rws:

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?

[rws]

I meant to write parent.pyx.

[SimonKing]

Replying to rws:

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

._is_coercion_cached() is related with self._coerce_from_hash. I think it would be a good idea to create a single underscore method that allows to clear self._coerce_from_hash and self._convert_from_hash. Proposed name: _clear_coerce_caches.

For sanity, I think it would make sense to open a separate ticket for it, which I will do in a few minutes. Then, you can use the new ticket as a dependency and add a ._clear_coerce_caches() in front of any test that requires a pristine state of the coercion cache.

[SimonKing]

Aha, I just found that Parent.init_coerce(False) would do the job. However, it is a cdef method and hence cannot be directly called.

[rws]

If you rely on patchbots for review the pynac upgrade needs to be merged by the maintainer first.

---

New commits:

​f5b3de4	24668: version/chzksum
​67f9ddc	24668: doctest fixes
​e7e68c7	Add method to remove some coercions/conversions from cache
​cd359c3	Merge branch 'u/SimonKing/add_a_method_to_clear_coercion_caches_of_a_parent' of git://trac.sagemath.org/sage into tmp16
​0ee9478	Merge branch 'u/rws/don_t_call_maxima_with_no_variable_symbolic_relation_tests' of git://trac.sagemath.org/sage into tmp16
​e78e84b	24658: doctest fixes

[rws]

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

[embray]

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

[embray]

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]

Ticket retargeted after milestone closed

[mkoeppe]

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

[mkoeppe]

This is a clear improvement.