evaluate context manager #2917

Merged
merged 17 commits into from Apr 4, 2014

Conversation

Projects
None yet
6 participants
Member

mrocklin commented Feb 16, 2014

Sometimes we want to turn off automatic evaluation. We usually suggest that people make the following change

# Before
x + x

# After
Add(x, x, evaluate=False)

This is ugly and, more importantly, hard to do on large complex operations.

This PR sets the default value of the evaluate keyword to a globally accessible variable. It then introduces an evaluate context manager to manage that global variable

>>> from sympy.abc import x
>>> from sympy.core.operations import evaluate
>>> print x + x
2*x
>>> with evaluate(False):
...     print x + x
x + x
Member

mrocklin commented Feb 16, 2014

Also, I had to temporarily disable caching. Probably there is a clean way to avoid this (we also need to cache on the current value of global_evaluate[0]

Member

mrocklin commented Feb 16, 2014

This now works on a number of other cases, most notably Functions like sin, cos, log, etc....

Owner

asmeurer commented Feb 16, 2014

This would also make sympify(evaluate=False) much cleaner.

Owner

asmeurer commented Feb 16, 2014

Given the existing API, this is probably the cleanest way to do it. We may want to think about cleaner ways to perform the same action, like wrapper Hold-type objects or separate Add and AddOp. But it's nice that this API doesn't depend on the actual implementation, so we could change that later.

Member

mrocklin commented Feb 16, 2014

One issue that has come up is that I sometimes want some kinds of operations to evaluate but not others. For example, I'm pretty ok with 0 + x -> x. It would be nice to tell this to SymPy somehow. I can imagine evaluate(False) some day being replaced with evaluate(steps=3) meaning, don't evaluate until you're three steps down in the call chain, then evaluate however you like.

Or maybe we just don't evaluate anything and then go in after the fact similar to how simplify or fu works, deciding at each step what to do based on some objective function.

Member

mrocklin commented Feb 16, 2014

Case in point

In [1]: with evaluate(False):
   ...:     pprint(sin(x**2).diff(x))
   ...:     
 2 ⎛           2 ⎞    ⎛ 2⎞
x ⋅⎜0⋅log(x) + ──⎟⋅cos⎝x ⎠
   ⎜            1⎟        
   ⎝           x ⎠        
Owner

asmeurer commented Feb 16, 2014

Well, if you really want no evaluation, that may just be what you want. If you're teaching/learning how derivatives work, that kind of output can be valuable. SymPy already doesn't do any evaluation by default unless it's dead obvious, so you should expect almost all evaluate(False) output to look kind of stupid.

Owner

asmeurer commented Feb 16, 2014

Also, a heads up when playing with this: the printers are not always in-tune with unevaluated objects. They sometimes make assumptions that certain canonicalizations have taken place (this has been improved over the years, but I imagine there are still issues there). So you should always be aware that the objects may not actually be what the printers show them as.

Owner

asmeurer commented Feb 16, 2014

There have also been issues where the printers do some operation which re-evaluates the object. Just want to warn you before you spend an hour debugging something that ends up just being an issue with the printers.

@skirpichev skirpichev and 1 other commented on an outdated diff Feb 16, 2014

sympy/simplify/simplify.py
@@ -1472,6 +1475,8 @@ def collect_sqrt(expr, evaluate=True):
========
collect, collect_const, rcollect
"""
+ if evaluate is none:
Member

smichr commented Feb 16, 2014

That's a great reminder, Aaron. I also note that you can check for discrepancies with _aresame

>>> from sympy.core.basic import _aresame
>>> p = Pow(2,-1,evaluate=0)
>>> _aresame(p, S(str(p)))
False

@smichr smichr and 1 other commented on an outdated diff Feb 16, 2014

sympy/core/evaluate.py
@@ -0,0 +1,24 @@
+from contextlib import contextmanager
+
+
+global_evaluate = [True]
+
+
+@contextmanager
+def evaluate(x):
+ """
+
+ >>> from sympy.abc import x
@smichr

smichr Feb 16, 2014

Member
Examples
========

@skirpichev skirpichev and 2 others commented on an outdated diff Feb 16, 2014

sympy/simplify/simplify.py
@@ -3512,6 +3517,8 @@ def signsimp(expr, evaluate=True):
exp(-(x - y))
"""
+ if global_evaluate is None:
@skirpichev

skirpichev Feb 16, 2014

Contributor

btw, I see a lot of boilerplate code here. Probably, we can introduce some decorator to handle this?

@smichr

smichr Feb 16, 2014

Member

I was just thinking the same but I envisioned an ifNone function:

def ifNone(new, old):
    """ifNone(1, foo) -> 1 if foo is None else foo"""
    return new if old is None else old
@mrocklin

mrocklin Feb 16, 2014

Member

I'm happy with a decorator. For me this work is part of a bigger picture
that includes step by step manipulation and guided simplification as we did
in full trig simplification. For this work we do need some sort of
transformation on many small sympy functions.
On Feb 16, 2014 12:15 PM, "Sergey B Kirpichev" notifications@github.com
wrote:

In sympy/simplify/simplify.py:

@@ -3512,6 +3517,8 @@ def signsimp(expr, evaluate=True):
exp(-(x - y))

 """
  • if global_evaluate is None:

btw, I see a lot of boilerplate here. Probably, we can introduce some
decorator to handle this?

Reply to this email directly or view it on GitHubhttps://github.com/sympy/sympy/pull/2917/files#r9778510
.

Member

smichr commented Feb 16, 2014

ok with 0 + x -> x.

The handling of cls.identity happens before the evaluate flag is even consulted, in AssocOp.__new__ so you will never see cls.identity in an expression

>>> Add(x,S.Zero,evaluate=0)
x
>>> _.args
()
>>> Mul(x,S.One,evaluate=0).args
()

BTW, cool idea!

Member

mrocklin commented Feb 17, 2014

I've fixed the typos raised.

I have not yet implemented any particular context manager or ifNone function. I don't think we have a firm idea yet of what we really need. Conversation here is good.

I've removed the WIP label from the PR.

One thing missing is any sort of testing on this. After playing around a bit I discovered issues relating to FiniteSet. I suspect that more exist. If people are willing to download and play around that would be excellent.

How else can this idea be matured/improved?

Member

smichr commented Feb 17, 2014

It doesn't quite give the result that evaluating with evaluate=False would give in terms of treating the input expression atomically:

>>> with evaluate(False):
...  1+x+y
...
y + x + 1
>>> _.args
(x + 1, y)
>>> with evaluate(False):
...   x + y + x
...
x + x + y
>>> _.args
(x + y, x)
>>> Add(x,y,x,evaluate=0).args
(x, y, x)
Member

mrocklin commented Feb 17, 2014

Hrm, right. It's literally probably doing the following:

Add(Add(1, x, evaluate=False), y, evaluate=False)

Do we want to exclude tree flattening from evaluate=False? My guess is not for now but that in the future it would be interesting to think about a system in which we could specify the types of simplifications we want.

Owner

asmeurer commented Feb 18, 2014

You don't need ifNone. Just use evaluate = evaluate or global_evaluate[0].

Owner

asmeurer commented Feb 18, 2014

So I suspect that if you do

with evaluate(False):
    some_function(expr)

where some_function is any function that does a nontrivial calculation, like simplify or integrate, that things are going to break down terribly, because so many things (rightly) assume the invariants of canonicalized expressions.

This is a pretty good argument for unevaluated objects to have different classes Add(x, x, evaluate=False) should not be an Add, because then we can't say that an Add always satisfies some invariant (in this case, "each term, appears only once, where a 'term' is an expression minus a rational or float coefficient").

Owner

asmeurer commented Feb 18, 2014

Do we want to exclude tree flattening from evaluate=False?

I'd say so. That's the whole point of AssocOp (at least you'd guess so from the name). If it's going to remove identities even with evaluate=False, it might as well do this too.

Member

mrocklin commented Feb 18, 2014

Well, sometimes you *do *want to be able to construct the expression tree
((x + y) + z).

On Mon, Feb 17, 2014 at 4:54 PM, Aaron Meurer notifications@github.comwrote:

Do we want to exclude tree flattening from evaluate=False?

I'd say so. That's the whole point of AssocOp (at least you'd guess so
from the name). If it's going to remove identities even with
evaluate=False, it might as well do this too.

Reply to this email directly or view it on GitHubhttps://github.com/sympy/sympy/pull/2917#issuecomment-35337959
.

Member

mrocklin commented Feb 19, 2014

Yup, things definitely break down. What should we do here? Include this with a strong warning or not include it?

hargup referenced this pull request Feb 19, 2014

Open

Set Expressions #2721

2 of 4 tasks complete
Owner

asmeurer commented Feb 20, 2014

It's a neat toy, but not a real solution. A real solution has Add(x, x, evaluate=False) as a class that isn't Add.

As I noted above, the nice thing about this is that we can change the implementation without changing the interface. So it probably wouldn't be too bad to include this.

Owner

asmeurer commented Feb 20, 2014

Travis fails.

Member

mrocklin commented Feb 20, 2014

I agree with you about the toy-like nature of it. One could hope that someone picks it up in the future. Its a weird tool in a space where I think we need more work. Maybe someone can figure out what to do with it.

A real solution has Add(x, x, evaluate=False) as a class that isn't Add.

I'm not convinced that this is the only way. This is a long and separate conversation though.

Owner

asmeurer commented Feb 20, 2014

Its a weird tool in a space where I think we need more work.

I agree that a lot of people would like to have this. Let's add it but put the disclaimer on it.

I'm not convinced that this is the only way. This is a long and separate conversation though.

One way or another it has to be. Otherwise can't say anything about the invariants of Add. Right now you can write code like

if isinstance(expr, Add):
    for i in expr.args:
        if i == x:
            do_something()
            break

(that's a stupid example, but you get the idea). That isn't quite correct if we allow that x could appear in expr.args twice.

Member

mrocklin commented Feb 20, 2014

As much as I want to dive into this discussion I'm going to stick to my previous point:

This is a long and separate conversation though.

Maybe we should have a wiki page on this topic (maybe we already do?) Some sort of Q&A format might be appropriate. Maybe Challenge & Response?

Owner

asmeurer commented Feb 20, 2014

Fair enough. I seem to remember that there is something about this on the wiki. I could be wrong, though.

Member

mrocklin commented Feb 20, 2014

Clearly we need a wiki page about wiki pages about long running disputes :)

On Wed, Feb 19, 2014 at 4:43 PM, Aaron Meurer notifications@github.comwrote:

Fair enough. I seem to remember that there is something about this on the
wiki. I could be wrong, though.

Reply to this email directly or view it on GitHubhttps://github.com/sympy/sympy/pull/2917#issuecomment-35570237
.

Owner

asmeurer commented Feb 20, 2014

I think that's about it. There really hasn't been any extended discussion with real concrete ideas to my memory (but then again I might only remember ideas if I liked them :)

Member

mrocklin commented Feb 28, 2014

I've added a warning to the evaluate docstring and merged with master to resolve conflicts.

@mrocklin mrocklin evaluate=None means trust global, not False
This was assumed in a test.  I'm unassuming it.  Hopefully no one cares
7a5b245
Member

mrocklin commented Feb 28, 2014

I changed a bit of behavior in 7a5b245 . In particular Operation(*args, evaluate=None) means trust the global_evaluate value rather than being the same as Operation(*args, evaluate=False).

Owner

asmeurer commented Mar 1, 2014

Yes, that way seems better.

Member

mrocklin commented Mar 30, 2014

This passes tests. Is there anything else to be done here?

Owner

asmeurer commented Mar 30, 2014

Are there tests for this?

Member

mrocklin commented Mar 30, 2014

Turns out that there were tests in my hard drive but not in my repo. pushing now. I'll add a few more.

Member

mrocklin commented Apr 1, 2014

I believe that this is ready to merge. Tests pass (or timeout).

Owner

asmeurer commented Apr 2, 2014

Test evaluate(True)

@mrocklin mrocklin add evaluate(True) test
be49bb2
Member

mrocklin commented Apr 2, 2014

Owner

asmeurer commented Apr 4, 2014

OK, I've hassled you enough.

@asmeurer asmeurer added a commit that referenced this pull request Apr 4, 2014

@asmeurer asmeurer Merge pull request #2917 from mrocklin/with-evaluate
evaluate context manager
bc010a0

@asmeurer asmeurer merged commit bc010a0 into sympy:master Apr 4, 2014

0 of 2 checks passed

continuous-integration/travis-ci The Travis CI build could not complete due to an error
Details
default The Travis CI build could not complete due to an error
Details
Owner

asmeurer commented Apr 4, 2014

Can you add this to the release notes?

Member

mrocklin commented Apr 4, 2014

Done.

On Fri, Apr 4, 2014 at 12:44 PM, Aaron Meurer notifications@github.comwrote:

Can you add this to the release notes?

Reply to this email directly or view it on GitHubhttps://github.com/sympy/sympy/pull/2917#issuecomment-39603734
.

Owner

asmeurer commented Apr 12, 2014

I just noticed that the doctest for this function doesn't support Python 3 (for shame!). What's worrying is that the doctest runner didn't catch this. Do doctests not run on decorated functions?

Owner

asmeurer commented Apr 12, 2014

#7399 for the doctest fix. I didn't look into why it isn't running yet.

Member

debugger22 commented Apr 13, 2014

>>> a = sympy.Matrix([[1, 0], [0, 1]])
>>> with evaluate(False):
...     print a + a
...
Matrix([[2, 0], [0, 2]])

According to this PR, I think it should be.

[1, 0] + [0, 1]

I've also tried it on IPython with init_printing().

satels commented Apr 13, 2014

And

a = sympy.Matrix([[1, 0], [0, 1]])
b = sympy.Matrix([[1, 0], [0, 1]])

Add(a, b, evaluate=False)

Traceback:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/admin/workspace/krapp/env/src/sympy/sympy/core/basic.py", line 457, in __repr__
    return sstr(self, order=None)
  File "/Users/admin/workspace/krapp/env/src/sympy/sympy/printing/str.py", line 726, in sstr
    s = p.doprint(expr)
  File "/Users/admin/workspace/krapp/env/src/sympy/sympy/printing/printer.py", line 233, in doprint
    return self._str(self._print(expr))
  File "/Users/admin/workspace/krapp/env/src/sympy/sympy/printing/printer.py", line 257, in _print
    return getattr(self, printmethod)(expr, *args, **kwargs)
  File "/Users/admin/workspace/krapp/env/src/sympy/sympy/printing/str.py", line 52, in _print_Add
    terms = self._as_ordered_terms(expr, order=order)
  File "/Users/admin/workspace/krapp/env/src/sympy/sympy/printing/printer.py", line 271, in _as_ordered_terms
    return expr.as_ordered_terms(order=order)
  File "/Users/admin/workspace/krapp/env/src/sympy/sympy/core/expr.py", line 806, in as_ordered_terms
    terms, gens = self.as_terms()
  File "/Users/admin/workspace/krapp/env/src/sympy/sympy/core/expr.py", line 835, in as_terms
    coeff, _term = term.as_coeff_Mul()
  File "/Users/admin/workspace/krapp/env/src/sympy/sympy/matrices/matrices.py", line 3039, in __getattr__
    "%s has no attribute %s." % (self.__class__.__name__, attr))
AttributeError: ImmutableMatrix has no attribute as_coeff_Mul.
Owner

asmeurer commented Apr 13, 2014

You have to use MatAdd with matrices.

satels commented Apr 13, 2014

Thank you!

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