sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.47k stars 485 forks source link

rewrite Expression.__nonzero__() #19040

Open rwst opened 9 years ago

rwst commented 9 years ago

Symbolic expressions may be part of type-neutral computations, e.g. matrices, polynomials. Developers do not expect proof machinery to crank up when writing if x!=0, but this is just what happens. So bool(x1!=x2) should be changed to mean not (x1-x2).is_trivial_zero() for symbolic x. The ticket should provide a different interface for cases requiring simplification/proof:

This ticket will implement the new behaviour of bool(rel) and put all other functionality of ex.__nonzero__() into holds() and ex.is_zero(simplify=True).

See also #19162.

CC: @nexttime @behackl @kcrisman @eviatarbach

Component: symbolics

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

videlec commented 9 years ago
comment:1

Be careful that in a lot of Sage place there are

def my_generic_function(x):
    if not x:
        ...
    else:
        ...

or with not x replaced by x.is_zero().

So this change would also implies to change all of these in a uniform way.

rwst commented 9 years ago
comment:2

Replying to @videlec:

if not x:

bool(not x) calls PyObject_IsTrue which calls Expression.__nonzero__ which atm tries to prove that x is nonzero

or with not x replaced by x.is_zero().

This calls PyObject_IsTrue as well.

So this change would also implies to change all of these in a uniform way.

Right, Expression.__nonzero__ will then call x.is_trivial_zero() and everyone who wants a proof needs a dfferent method.

rwst commented 9 years ago
comment:3

It's not too bad. Catching != and == in __nonzero__ and comparing trivially yields only a few dozen doctest fails in symbolic and calculus, mainly from bool(...). No fail in src/doc.

videlec commented 9 years ago
comment:4

I agree that the change would be actually good (for the reason why you created this ticket).

Thoug, for symbolic expression we want to create equations and check their validity

sage: cos(x)**2 + sin(x)**2 == 1
cos(x) == sin(x)
sage: bool(_)
True

The above will not be enough anymore. What would be the new way of checking? This needs to be emphasized a lot in the documentation as it is backward incompatible. And I guess it is worth a thread on sage-devel. Not necessarily right now, it is always good to have concrete propositions.

You should also have a look to sage/tests/* where I am sure some of the things are broken.

rwst commented 9 years ago
comment:5

Replying to @videlec:

Thoug, for symbolic expression we want to create equations and check their validity

sage: cos(x)**2 + sin(x)**2 == 1
cos(x) == sin(x)
sage: bool(_)
True

The above will not be enough anymore. What would be the new way of checking?

sage: satisfiable(_)
True

This is a long-standing omission, and it would resolve conceptual problems of #17700. It would use #19000 and, if that finds no solution, Maxima as before. SMT solvers can also give a satisfying x in case of satisfiability, but no full solution which is the task of solve.

This needs to be emphasized a lot in the documentation as it is backward incompatible. And I guess it is worth a thread on sage-devel. Not necessarily right now, it is always good to have concrete propositions.

You should also have a look to sage/tests/* where I am sure some of the things are broken.

Three fails.

rwst commented 9 years ago
comment:6

Replying to @rwst:

It would use #19000

Could. #19000 is not a necessity.

rwst commented 9 years ago
comment:7

Actually, proving equality would need quantifiers like:

sage: satisfiable(_, for_all(x))
True
rwst commented 9 years ago

Description changed:

--- 
+++ 
@@ -1 +1,6 @@
-Symbolics may be part of type-neutral computations, e.g. matrices, rings. Developers do not expect proof machinery to crank up when writing `if x!=0`, so `bool(x1!=x2)` should mean `not (x1-x2).is_trivial_zero()` for symbolic `x`. Instead of `Expression.__nonzero__` this ticket should provide a different interface for cases requiring simplification/proof.
+Symbolics may be part of type-neutral computations, e.g. matrices, rings. Developers do not expect proof machinery to crank up when writing `if x!=0`, so `bool(x1!=x2)` should mean `not (x1-x2).is_trivial_zero()` for symbolic `x`. This ticket should provide a different interface for cases requiring simplification/proof:
+* `bool(rel)` equivalent to `(not)(LHS-RHS).is_trivial_zero()` for ==,!= ; and an exception with maybe hint to the following for <,>,<=,>=
+* `satisfiable(rel)` returning `(Yes,example)/No/Undecidable/NotImplemented`
+* `solve(rel)` in case of `satisfiable=Yes` returning the full solution set
+* `prove(rel)` showing more or less steps of simplification (which is out of reach for the moment)
+
rwst commented 9 years ago

Description changed:

--- 
+++ 
@@ -1,6 +1,7 @@
 Symbolics may be part of type-neutral computations, e.g. matrices, rings. Developers do not expect proof machinery to crank up when writing `if x!=0`, so `bool(x1!=x2)` should mean `not (x1-x2).is_trivial_zero()` for symbolic `x`. This ticket should provide a different interface for cases requiring simplification/proof:
 * `bool(rel)` equivalent to `(not)(LHS-RHS).is_trivial_zero()` for ==,!= ; and an exception with maybe hint to the following for <,>,<=,>=
 * `satisfiable(rel)` returning `(Yes,example)/No/Undecidable/NotImplemented`
+* `truth(rel, (x,S1), (y,S1)...)` equivalent to `satisfiable(rel)` for all `x,y...` in `S1,S2,...`
 * `solve(rel)` in case of `satisfiable=Yes` returning the full solution set
 * `prove(rel)` showing more or less steps of simplification (which is out of reach for the moment)
rwst commented 9 years ago

Description changed:

--- 
+++ 
@@ -1,7 +1,8 @@
-Symbolics may be part of type-neutral computations, e.g. matrices, rings. Developers do not expect proof machinery to crank up when writing `if x!=0`, so `bool(x1!=x2)` should mean `not (x1-x2).is_trivial_zero()` for symbolic `x`. This ticket should provide a different interface for cases requiring simplification/proof:
+Symbolics may be part of type-neutral computations, e.g. matrices, rings. Developers do not expect proof machinery to crank up when writing `if x!=0`, so `bool(x1!=x2)` should mean `not (x1-x2).is_trivial_zero()` for symbolic `x`. We should provide a different interface for cases requiring simplification/proof:
 * `bool(rel)` equivalent to `(not)(LHS-RHS).is_trivial_zero()` for ==,!= ; and an exception with maybe hint to the following for <,>,<=,>=
 * `satisfiable(rel)` returning `(Yes,example)/No/Undecidable/NotImplemented`
 * `truth(rel, (x,S1), (y,S1)...)` equivalent to `satisfiable(rel)` for all `x,y...` in `S1,S2,...`
 * `solve(rel)` in case of `satisfiable=Yes` returning the full solution set
 * `prove(rel)` showing more or less steps of simplification (which is out of reach for the moment)

+This ticket will implement the new behaviour of `bool(rel)` and put all other functionality of `ex.__nonzero__()` into `ex.satisfiable(rel)`.
rwst commented 9 years ago

Description changed:

--- 
+++ 
@@ -5,4 +5,6 @@
 * `solve(rel)` in case of `satisfiable=Yes` returning the full solution set
 * `prove(rel)` showing more or less steps of simplification (which is out of reach for the moment)

-This ticket will implement the new behaviour of `bool(rel)` and put all other functionality of `ex.__nonzero__()` into `ex.satisfiable(rel)`.
+This ticket will implement the new behaviour of `bool(rel)` and put all other functionality of `ex.__nonzero__()` into `ex.satisfiable()`.
+
+See also #19162.
rwst commented 9 years ago

Dependencies: #18980

rwst commented 9 years ago

Branch: u/rws/defuse_bool_x0performance_bomb

rwst commented 9 years ago

Commit: d94dec4

rwst commented 9 years ago

Author: Ralf Stephan

rwst commented 9 years ago

Changed dependencies from #18980 to none

rwst commented 9 years ago

New commits:

d94dec419040: draft, can't go further without 18980
rwst commented 9 years ago

Changed branch from u/rws/defuse_bool_x0performance_bomb to u/rws/19040

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from d94dec4 to 800ec56

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

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

f52dbd2missing void declaration of Cython function causes unexpected crashes
f67f47f19040 uncovers hidden calls of Expression.__nonzero__, fixed
38e983b19040: handle numerics together with constants
800ec5619040: draft3
rwst commented 9 years ago

Description changed:

--- 
+++ 
@@ -1,7 +1,7 @@
 Symbolics may be part of type-neutral computations, e.g. matrices, rings. Developers do not expect proof machinery to crank up when writing `if x!=0`, so `bool(x1!=x2)` should mean `not (x1-x2).is_trivial_zero()` for symbolic `x`. We should provide a different interface for cases requiring simplification/proof:
 * `bool(rel)` equivalent to `(not)(LHS-RHS).is_trivial_zero()` for ==,!= ; and an exception with maybe hint to the following for <,>,<=,>=
 * `satisfiable(rel)` returning `(Yes,example)/No/Undecidable/NotImplemented`
-* `truth(rel, (x,S1), (y,S1)...)` equivalent to `satisfiable(rel)` for all `x,y...` in `S1,S2,...`
+* `holds(rel, (x,S1), (y,S1)...)` equivalent to `satisfiable(rel)` for all `x,y...` in `S1,S2,...`
 * `solve(rel)` in case of `satisfiable=Yes` returning the full solution set
 * `prove(rel)` showing more or less steps of simplification (which is out of reach for the moment)
rwst commented 9 years ago

Description changed:

--- 
+++ 
@@ -1,10 +1,10 @@
-Symbolics may be part of type-neutral computations, e.g. matrices, rings. Developers do not expect proof machinery to crank up when writing `if x!=0`, so `bool(x1!=x2)` should mean `not (x1-x2).is_trivial_zero()` for symbolic `x`. We should provide a different interface for cases requiring simplification/proof:
-* `bool(rel)` equivalent to `(not)(LHS-RHS).is_trivial_zero()` for ==,!= ; and an exception with maybe hint to the following for <,>,<=,>=
+Symbolic expressions may be part of type-neutral computations, e.g. matrices, polynomials. Developers do not expect proof machinery to crank up when writing `if x!=0`, but this is just what happens. So `bool(x1!=x2)` should be changed to mean `not (x1-x2).is_trivial_zero()` for symbolic `x`. The ticket should provide a different interface for cases requiring simplification/proof:
+* `bool(rel)` equivalent to `(not)(LHS-RHS).is_trivial_zero()` for ==,!= ; and for <, >, <=, >= the result follows the print order of lhs and rhs
 * `satisfiable(rel)` returning `(Yes,example)/No/Undecidable/NotImplemented`
 * `holds(rel, (x,S1), (y,S1)...)` equivalent to `satisfiable(rel)` for all `x,y...` in `S1,S2,...`
 * `solve(rel)` in case of `satisfiable=Yes` returning the full solution set
 * `prove(rel)` showing more or less steps of simplification (which is out of reach for the moment)

-This ticket will implement the new behaviour of `bool(rel)` and put all other functionality of `ex.__nonzero__()` into `ex.satisfiable()`.
+This ticket will implement the new behaviour of `bool(rel)` and put all other functionality of `ex.__nonzero__()` into `ex.satisfiable()` and `ex.holds()`.

 See also #19162.
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from 800ec56 to adab9a7

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

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

7a3a4d219040: infinity handled by Pynac
7c6a8aa19040: contradicts() now uses satisfiable()
0d4e2a119040: prep satisfiable()
d8971edMerge branch 'develop' into t/19040/19040
adab9a719040: draft4
rwst commented 9 years ago

Dependencies: #17984

rwst commented 9 years ago

Changed dependencies from #17984 to #19256

rwst commented 9 years ago

Changed branch from u/rws/19040 to u/rws/19040-1

rwst commented 9 years ago

Changed dependencies from #19256 to none

rwst commented 9 years ago

Changed commit from adab9a7 to 5f4ffc0

rwst commented 9 years ago
comment:23

It may not be possible to get the desired behaviour in the structure/parent.pyx doctests, so we oblige Parent.__contains__ by throwing an exception if lhs-rhs contains inexact ring elements. This way inclusion in those rings (i.e., pi in CC == True) is always guaranteed as before.


New commits:

5f4ffc019040: draft
videlec commented 9 years ago
comment:24

A small remark: From the documentation of the method holds

If Sage knows exactly that the relation is
undecidable it will throw an ``AttributeError``.

For one relation there always is an algorithm which is either return True or return False. What is not possible is to design an algorithm whose input is an equation and answers the validity of the input. Your sentence makes no sense. A right formulation would be

If Sage does not know if the equation is valid it will
throw a ``NotImplementedError``. Note that the validity
of equations is an undecidable problem. Hence there will
always be instances for which such error is raised.

(AttributeError makes no sense here).

rwst commented 9 years ago

Changed branch from u/rws/19040-1 to u/rws/19040-2

rwst commented 9 years ago
comment:26

This should be it. Please review.


New commits:

fd2e72319040: core of rewrite Expression.__nonzero__()
5a0c829code changes in consequence of 19040
fdd148a19040: doctest changes
rwst commented 9 years ago

Changed commit from 5f4ffc0 to fdd148a

rwst commented 9 years ago

Description changed:

--- 
+++ 
@@ -1,8 +1,10 @@
 Symbolic expressions may be part of type-neutral computations, e.g. matrices, polynomials. Developers do not expect proof machinery to crank up when writing `if x!=0`, but this is just what happens. So `bool(x1!=x2)` should be changed to mean `not (x1-x2).is_trivial_zero()` for symbolic `x`. The ticket should provide a different interface for cases requiring simplification/proof:
 * `bool(rel)` equivalent to `(not)(LHS-RHS).is_trivial_zero()` for ==,!= ; and for <, >, <=, >= the result follows the print order of lhs and rhs
+* `rel.is_zero(simplify=False)` (default) calling the fast `bool(rel)`
+* `rel.is_zero(simplify=True)` attempting simplification/proof
 * `satisfiable(rel)` returning `(Yes,example)/No/Undecidable/NotImplemented`
+* `solve(rel)` in case of `satisfiable=Yes` returning the full solution set
 * `holds(rel, (x,S1), (y,S1)...)` equivalent to `satisfiable(rel)` for all `x,y...` in `S1,S2,...`
-* `solve(rel)` in case of `satisfiable=Yes` returning the full solution set
 * `prove(rel)` showing more or less steps of simplification (which is out of reach for the moment)

 This ticket will implement the new behaviour of `bool(rel)` and put all other functionality of `ex.__nonzero__()` into `ex.satisfiable()` and `ex.holds()`.
rwst commented 9 years ago

Description changed:

--- 
+++ 
@@ -7,6 +7,6 @@
 * `holds(rel, (x,S1), (y,S1)...)` equivalent to `satisfiable(rel)` for all `x,y...` in `S1,S2,...`
 * `prove(rel)` showing more or less steps of simplification (which is out of reach for the moment)

-This ticket will implement the new behaviour of `bool(rel)` and put all other functionality of `ex.__nonzero__()` into `ex.satisfiable()` and `ex.holds()`.
+This ticket will implement the new behaviour of `bool(rel)` and put all other functionality of `ex.__nonzero__()` into `ex.is_zero(simplify=True)`.

 See also #19162.
rwst commented 9 years ago

Description changed:

--- 
+++ 
@@ -1,12 +1,12 @@
 Symbolic expressions may be part of type-neutral computations, e.g. matrices, polynomials. Developers do not expect proof machinery to crank up when writing `if x!=0`, but this is just what happens. So `bool(x1!=x2)` should be changed to mean `not (x1-x2).is_trivial_zero()` for symbolic `x`. The ticket should provide a different interface for cases requiring simplification/proof:
-* `bool(rel)` equivalent to `(not)(LHS-RHS).is_trivial_zero()` for ==,!= ; and for <, >, <=, >= the result follows the print order of lhs and rhs
-* `rel.is_zero(simplify=False)` (default) calling the fast `bool(rel)`
-* `rel.is_zero(simplify=True)` attempting simplification/proof
+* `bool(rel)` equivalent to `(not)(LHS-RHS).is_trivial_zero()` for ==,!= ; and for <, >, <=, >= the result follows alpha order of lhs and rhs
 * `satisfiable(rel)` returning `(Yes,example)/No/Undecidable/NotImplemented`
 * `solve(rel)` in case of `satisfiable=Yes` returning the full solution set
-* `holds(rel, (x,S1), (y,S1)...)` equivalent to `satisfiable(rel)` for all `x,y...` in `S1,S2,...`
+* `is(rel)` attempting simplification/proof, returning `True`/`False`, throwing `NotImplementedError` 
+* `ex.is_zero(simplify=False)` (default) calling the fast `bool(ex==0)`
+* `ex.is_zero(simplify=True)` attempting simplification/proof
 * `prove(rel)` showing more or less steps of simplification (which is out of reach for the moment)

-This ticket will implement the new behaviour of `bool(rel)` and put all other functionality of `ex.__nonzero__()` into `ex.is_zero(simplify=True)`.
+This ticket will implement the new behaviour of `bool(rel)` and put all other functionality of `ex.__nonzero__()` into `is()` and `ex.is_zero(simplify=True)`.

 See also #19162.
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

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

f30fe23Merge branch 'develop' into t/19040/19040-2
ba8824e19040: add dedicated is_zero()
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from fdd148a to ba8824e

rwst commented 9 years ago

Description changed:

--- 
+++ 
@@ -2,11 +2,11 @@
 * `bool(rel)` equivalent to `(not)(LHS-RHS).is_trivial_zero()` for ==,!= ; and for <, >, <=, >= the result follows alpha order of lhs and rhs
 * `satisfiable(rel)` returning `(Yes,example)/No/Undecidable/NotImplemented`
 * `solve(rel)` in case of `satisfiable=Yes` returning the full solution set
-* `is(rel)` attempting simplification/proof, returning `True`/`False`, throwing `NotImplementedError` 
+* `holds(rel)` attempting simplification/proof, returning `True`/`False`, throwing `NotImplementedError` 
 * `ex.is_zero(simplify=False)` (default) calling the fast `bool(ex==0)`
-* `ex.is_zero(simplify=True)` attempting simplification/proof
+* `ex.is_zero(simplify=True)` attempting simplification/proof by calling `ex==0`.holds()
 * `prove(rel)` showing more or less steps of simplification (which is out of reach for the moment)

-This ticket will implement the new behaviour of `bool(rel)` and put all other functionality of `ex.__nonzero__()` into `is()` and `ex.is_zero(simplify=True)`.
+This ticket will implement the new behaviour of `bool(rel)` and put all other functionality of `ex.__nonzero__()` into `holds()` and `ex.is_zero(simplify=True)`.

 See also #19162.
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

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

857dc9c19040: add dedicated is_zero()
595080cMerge branch 'u/rws/19040-2' of trac.sagemath.org:sage into t/19040/19040-2
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 9 years ago

Changed commit from ba8824e to 595080c

rwst commented 9 years ago
comment:33

Patchbot failures in hyperbolic space.

rwst commented 9 years ago
comment:34

This code cannot be separated from #19312 so I included it there. I'll let this ticket stay here for the description unless someone objects.

rwst commented 9 years ago

Changed branch from u/rws/19040-2 to none

rwst commented 9 years ago

Changed commit from 595080c to none

rwst commented 9 years ago

Changed author from Ralf Stephan to none

jdemeyer commented 9 years ago
comment:35

Replying to @rwst:

This code cannot be separated from #19312

Please explain why.

rwst commented 9 years ago
comment:36

Replying to @jdemeyer:

Replying to @rwst:

This code cannot be separated from #19312

Please explain why.

Correction: an advanced version of this code cannot be separated from #19312. So, this is already obsolete. If, however, #19312 merge is not in the forseeable future I'll consider presenting this branch for review again.

jdemeyer commented 9 years ago
comment:37

I would much prefer to separate #19040 from #19312, mainly to make the review easier (reviewing a package upgrade plus a huge number of Sage library changes is difficult).