Opened 4 years ago

Closed 17 months ago

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

Reported by: Owned by: rws major sage-9.2 symbolics SimonKing Ralf Stephan Simon King, Matthias Koeppe N/A e78e84b e78e84bfcd13292e30969c2be007e71337810c87

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.

### comment:1 Changed 4 years ago by rws

• Branch set to u/rws/don_t_call_maxima_with_no_variable_symbolic_relation_tests

### comment:2 Changed 4 years ago by rws

• Commit set to 0a29d407464bbff73b3de64b2f4e2d4f90b6d8f4
• Description modified (diff)
• Status changed from new to needs_review

New commits:

 ​0a29d40 `24658: Don't call Maxima with no-variable symbolic relation tests`

### comment:3 Changed 4 years ago by rws

• Authors set to Ralf Stephan

### comment:5 follow-ups: ↓ 7 ↓ 9 Changed 4 years ago by 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

```diff --git a/src/sage/symbolic/expression.pyx b/src/sage/symbolic/expression.pyx
index 7130898..0f943d9 100644```
 a 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.

### comment:6 Changed 4 years ago by 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.

### comment:7 in reply to: ↑ 5 Changed 4 years ago by rws

Also, I wonder about that change:

• ## src/sage/symbolic/expression.pyx

```diff --git a/src/sage/symbolic/expression.pyx b/src/sage/symbolic/expression.pyx
index 7130898..0f943d9 100644```
 a 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.

Last edited 4 years ago by rws (previous) (diff)

### comment:8 Changed 4 years ago by git

• Commit changed from 0a29d407464bbff73b3de64b2f4e2d4f90b6d8f4 to 17827ea66659d4bf15bcd20c47b1f92d7c1e7418

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

 ​17827ea `24658: address reviewer's comments`

### comment:9 in reply to: ↑ 5 ; follow-up: ↓ 10 Changed 4 years ago by rws

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

### comment:10 in reply to: ↑ 9 Changed 4 years ago by 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`.

### comment:11 follow-up: ↓ 12 Changed 4 years ago by 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
```
Version 1, edited 4 years ago by SimonKing (previous) (next) (diff)

### comment:12 in reply to: ↑ 11 Changed 4 years ago by rws

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

### comment:13 Changed 4 years ago by git

• Commit changed from 17827ea66659d4bf15bcd20c47b1f92d7c1e7418 to 590600b9723c2cc8d8b985097725854cb486390a

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

 ​590600b `24658: if test_relation fails try again simplified`

### comment:14 Changed 4 years ago by rws

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

### comment:15 Changed 4 years ago by 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.

### comment:16 Changed 4 years ago by 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`.

### comment:17 Changed 4 years ago by rws

• Dependencies set to #24668

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

### comment:18 follow-up: ↓ 22 Changed 4 years ago by 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...

### comment:19 follow-up: ↓ 20 Changed 4 years ago by 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?

### comment:20 in reply to: ↑ 19 Changed 4 years ago by 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?

### comment:21 Changed 4 years ago by rws

I meant to write `parent.pyx`.

### comment:22 in reply to: ↑ 18 Changed 4 years ago by SimonKing

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.

### comment:23 Changed 4 years ago by SimonKing

• Dependencies changed from #24668 to #24668 #24673

### comment:24 Changed 4 years ago by 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.

### comment:25 Changed 4 years ago by rws

• Branch changed from u/rws/don_t_call_maxima_with_no_variable_symbolic_relation_tests to u/rws/24658

### comment:26 Changed 4 years ago by rws

• Commit changed from 590600b9723c2cc8d8b985097725854cb486390a to e78e84bfcd13292e30969c2be007e71337810c87

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`

### comment:27 Changed 4 years ago by rws

• Status changed from needs_review to needs_work

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

### comment:28 Changed 4 years ago by rws

• Status changed from needs_work to needs_review

### comment:29 Changed 3 years ago by chapoton

• Dependencies #24668 #24673 deleted
• Milestone changed from sage-8.2 to sage-8.7

### comment:30 Changed 3 years ago by embray

• Milestone changed from sage-8.7 to sage-8.8

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

### comment:31 Changed 3 years ago by embray

• Milestone changed from sage-8.8 to sage-8.9

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

### comment:32 Changed 2 years ago by embray

• Milestone changed from sage-8.9 to sage-9.1

Ticket retargeted after milestone closed

### comment:33 Changed 21 months ago by mkoeppe

• Milestone changed from sage-9.1 to sage-9.2

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

### comment:34 Changed 18 months ago by mkoeppe

• Reviewers set to Simon King, Matthias Koeppe
• Status changed from needs_review to positive_review

This is a clear improvement.

### comment:35 Changed 17 months ago by vbraun

• Branch changed from u/rws/24658 to e78e84bfcd13292e30969c2be007e71337810c87
• Resolution set to fixed
• Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.