TODO: rebase Addressing Piecewise issues #1009

Open
wants to merge 13 commits into
from

Projects

None yet

5 participants

@flacjacket
Member

This push is to address a couple open issues with Piecewise and its syntax, notably changing the otherwise syntax. Other major changes with this commit are getting rid of the ExprCondPair class and changing how evaluation is handled. The use of True as the condition to specify otherwise conditions is deprecated. Additionally, the use of numbers as conditions is deprecated and the use of Interval's as conditions is dropped entirely. To implement the same functionality, either the '.as_relational()' or '.contains()' methods of Interval can be used.

As before, evaluation strips any bools out of the args by dropping expressions with False, but now expressions with condition=True are promoted to the otherwise statement by evaluation. Evaluation is not performed by default, but it is called when subs is called to allow for evaluation in that case. When bools end up in the args and evaluate is the default value of False, a deprecation warning is raised and evaluate is set to True to get clear the args of the bool.

The properties '.exprcondpairs' and '.otherwise' are implemented to easily access the expr/cond pairs and otherwise statement, respectively.

TODO:

This addresses the following issues:

2101 latex printing with piecewise function : reversed expr and cond

2567 Piecewise does not work when not given an "otherwise" condition

2626 Piecewise should use a different syntax for "otherwise"

2710 Cannot simplify Piecewise

2726 Piecewise((1,Interval(0,1,False,True)),(0,True)) syntax should not be allowed

Also, the following also seems like it has been handled, but not closed:

1216 series expansion of piecewise fails

@asmeurer
Member

Thanks for working on this. I started to work on this a while back, but I ran into problems with _eval_leading_term. Did you encounter those issues?

@asmeurer asmeurer and 1 other commented on an outdated diff Jan 22, 2012
sympy/functions/elementary/piecewise.py
- Usage:
+ - Expr/cond pairs where the cond is explicitly False will be removed.
+ - Expr/cond pairs which cannot be determined, e.g. x < 1 for a Symbol x,
+ are returned symbolically.
+ - For expr/cond pairs where the cond is explicitly True, e.g. 1 < 2,
+ and there are no undetermined expr/cond pairs appearing befare it,
@flacjacket
flacjacket Jan 22, 2012 Member

Thanks, fixed

@asmeurer
Member

You've made things awfully complicated here? Why don't you just do what was suggested in issue 2626? Namely, use the syntax, Piecewise((expr1, cond1), ..., (exprn, condn), [otherwise]) (where the square brackets indicate that the argument is optional).

@asmeurer
Member

Ah, well I see that that syntax does work in your branch. I suppose the complicated .args is necessary to support the old syntax as well (?) I think that .args should just use the new syntax, and all other supported syntaxes should be canonicalized to it for .args (similar to what Integral does).

By the way, I also think we ought to deprecate the old syntax, as it's very confusing.

@asmeurer asmeurer and 1 other commented on an outdated diff Jan 22, 2012
sympy/functions/elementary/piecewise.py
return None
- def doit(self, **hints):
- """
- Evaluate this piecewise function.
- """
- newargs = []
- for e, c in self.args:
- if hints.get('deep', True):
- if isinstance(e, Basic):
- e = e.doit(**hints)
- if isinstance(c, Basic):
- c = c.doit(**hints)
- newargs.append((e, c))
- return Piecewise(*newargs)
+ @property
+ def ecs(self):
@asmeurer
asmeurer Jan 22, 2012 Member

This needs a better name.

@flacjacket
flacjacket Jan 22, 2012 Member

The most explanatory it could be is something like exprcondpairs, but that may be a bit verbose.

@asmeurer
asmeurer Jan 22, 2012 Member

That doesn't sound too verbose to me. It's better than ecs, which is cryptic.

@flacjacket
Member

The first commit I don't think makes things terribly complicated, it mostly just adds the ability to have the otherwise expression in addition to adding the .ecs and .otherwise properties to easily get out the expr/cond pairs and otherwise statements, since the args can no longer be iterated as 'e, c in args'.

The second commit makes things more complicated as it changes the canonical args to allow evaluate to be maintained between things like subs. The more I think about it, the more I think this might not be necessary, as the only use is when you want to do something like Piecewise((x,x>0), evaluate=False).subs(x,1) and want it to return a Piecewise rather than just returning 1.

As for the old syntax, I'm not sure that can really be deprecated. or at least we can't really raise warnings, as there shouldn't be anything wrong with having True as a condition, especially since there's no way to tell a condition 1>0 or (x>0).subs(x,1) apart from True. I changed all the instances in the code to use the new otherwise syntax and changed the documentation to reflect this.

In the same vein, in issue 2626, you noted allowing a 1-tuple for an otherwise statement, I'm not entirely sure if this is necessary, but I included it as a possibility, that may be leading to an increased mess in some of the code.

I had the same problem with _eval_leading_term. Function tried to do _eval_leading_term on args[0], but since for a Piecewise that's a Tuple, it fails. What I implemented was to add all the expressions, including otherwise, together and return _eval_leading_term of that. I'm not sure if that would be the desired behavior for a Piecewise function or if it should take the expression when the variable goes to 0, but it works in as much as the old tests still pass.

@asmeurer
Member

This needs to be merged or rebased with master. The issue is most likely the branch I recently pushed in that refactors the inequality classes.

@asmeurer
Member

Right. To be more clear, the complication is specifically in .args, and as a result in anything that parses .args, and the code that builds it. .args should just be ((expr, cond), ..., otherwise).

The more I think about it, the more I think this might not be necessary, as the only use is when you want to do something like Piecewise((x,x>0), evaluate=False).subs(x,1) and want it to return a Piecewise rather than just returning 1.

I think the solution here is to keep 1 > 0 from being evaluated. You will need to implement the evaluate keyword in the new inequality classes. Then Piecewise can just pass the evaluate flag to that (or do you think it would be cleaner to not do that automatically and require the user to do it?).

In the same vein, in issue 2626, you noted allowing a 1-tuple for an otherwise statement, I'm not entirely sure if this is necessary, but I included it as a possibility, that may be leading to an increased mess in some of the code.

This is an added source of complication. It is not necessary to support this syntax. We decided against it.

I had the same problem with _eval_leading_term. Function tried to do _eval_leading_term on args[0], but since for a Piecewise that's a Tuple, it fails. What I implemented was to add all the expressions, including otherwise, together and return _eval_leading_term of that. I'm not sure if that would be the desired behavior for a Piecewise function or if it should take the expression when the variable goes to 0, but it works in as much as the old tests still pass.

@ness01 or someone who knows more about what that does will have to comment. Is it OK for _eval_leading_term to "overestimate", so to speak? Getting the term at 0 may not be so easy, as the conditions could be more than one dimensional, and hence be symbolic after you substitute 0.

As for the old syntax, I'm not sure that can really be deprecated. or at least we can't really raise warnings, as there shouldn't be anything wrong with having True as a condition, especially since there's no way to tell a condition 1>0 or (x>0).subs(x,1) apart from True. I changed all the instances in the code to use the new otherwise syntax and changed the documentation to reflect this.

I do think the old syntax should be removed, as this is another source of complication in the code. Just trying to understand the bulleted list of what happens with the args in the docstring was making my head spin, and I already have a good idea of what it would do.

I don't see how to avoid all ambiguities without completely dropping the old syntax (this is why the syntax needed to be changed in the first place). However, if we do that without a deprecation cycle, then any user of Piecewise will have his Piecewise objects automatically evaluated at the otherwise condition, which may be surprising.

So I think we should try to raise a warning. Just do it whenever True is interpreted as the otherwise condition. If the user got True not because he wanted otherwise but because he wanted it to evaluate to that point, then he will need to be warned anyway. We can direct the users to avoid ambiguity during the deprecation period by adding nan as a manual otherwise condition. Then, interpret all True conditions before the otherwise as evaluation (I thought you were already doing this, but Piecewise((x,x>0), (y, 0 < 1), nan) is giving me the same thing as Piecewise((x,x>0), y)).

Do you see any problems with that plan?

@ness01
Contributor
ness01 commented Jan 22, 2012

As I have said elsewhere, I'm not quite certain why so many functions implement _eval_as_leading_term. According to the docstring (and the implementation!), as_leading_term may only be used on a result returned by Basic.series. Now, conceivably, this could return a piecewise. I think that the right thing for piecewise to do is to pass eval_leading_term through to its components.

But really I don't know for sure, and I am not even aware of any code that returns Piecewise in series, except for Piecewise itself. And, moreover, I don't think gruntz can handle piecewise anyway.

So in summary, my suggestion is to make as_leading_term behave just like series - simply return a piecewise with the same conditions and the inner functions replaced by their leading terms. But I am not at all confident this is right (since I know no use cases) and certainly did not write the code. Maybe @certik knows better.

@flacjacket
Member

It's currently rebased, but there are some test failures with the printing.

Do you see any problems with that plan?

That sounds good. I'll see what I can do to get that going.

As for the _eval_leading_term, I'll try making it a Piecewise and seeing that it doesn't fail until I hear otherwise.

@flacjacket
Member

@asmeurer So there is currently some test error in nseries that originates from somewhere in gruntz.py that I don't fully understand, but other than that, I've tried to fix this up, tho I'm not totally sure what exactly you were trying to say at some points, so please note if something is off.

I haven't changed any of the docstring for this, but here's the gist of it:

By default, evaluate isn't called and if there are any bool conds, it raises a warning and sets evaluate to True. eval then gets rid of any bools by discarding when False and promoting expressions to otherwise if their condition is True before otherwise. If subs is called, it is taken as an evaluation and eval is called to clear any bools.

With this Piecewise((x,x>0), (y, 0 < 1), nan) (which would be legal post-deprecation as Piecewise((x,x>0), (y, 0 < 1), nan, evaluate=True)) reduces to Piecewise((x,x>0), y), but only because evaluate is called. I'm not sure what you mean by:

Then, interpret all True conditions before the otherwise as evaluation

if not something like that. Do you mean the 0 < 1 is not trying to be called as an otherwise statement and thus should trigger evaluation, or because it is a True condition and not otherwise it should be kept as a expression/condition rather than evaluating to the otherwise statement? Should this use of a bool then warrant a deprecation warning?

I also put in deprecation for numbers as a condition, I see no reason why you'd want to use a number as a condition, but I could be wrong.

@asmeurer
Member

Please clean up your commit history.

@asmeurer
Member

Do you mean the 0 < 1 is not trying to be called as an otherwise statement and thus should trigger evaluation

Yes. If you just write Piecewise((x,x>0), (y, 0 < 1)), the y will be interpreted as otherwise. The workaround for users should be to add nan as the otherwise condition manually (this effectively shouldn't change anything, since conditions outside the range evaluate to nan by default when no otherwise is given).

I would give the warning for Piecewise((x,x>0), (y, 0 < 1)) but not Piecewise((x,x>0), (y, 0 < 1), nan), since the latter follows the new syntax, and is actually what we will recommend to people to avoid the warning (and make things correct).

@asmeurer
Member

Are numbers currently allowed as conditions? I think this shouldn't be allowed, for the same reason that intervals can't be used (they're not Booleans). Unless I'm misunderstanding what this means.

@flacjacket
Member

Alright, I think I get it.

And yes, numbers are currently allowed conditions, I can remove that as well.

@asmeurer
Member

From what I can see, they are just evaluated as booleans, i.e., nonzero means True and zero means False. Is that right?

Yeah, let's just remove that. I wouldn't even bother deprecating it.

@flacjacket
Member

Alright, I've finished cleaning up documentation, number conditions are removed and the deprecation warning is correctly implemented.

Also, the error that I was getting earlier was due to the as_leading_term. Doing abs(x - a).nseries(x, 1) adds a Piecewise with an Order. This ends up creating a new Order based off the leading term of the Piecewise, and thus it needs to be a normal function of the Symbol in question. To fix the test, I changed as_leading_term back to adding the expressions and taking the leading term of the sum.

@asmeurer
Member

SymPy Bot Summary: There were test failures.

@flacjacket: Please fix the test failures.

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sY7ZgKDA

Interpreter: /Library/Frameworks/Python.framework/Versions/3.2/bin/python3 (3.2.2-final-0)
Architecture: Darwin (64-bit)
Cache: yes
Test command: setup.py test
master hash: 9258bb9
branch hash: 97cc38361bdf20cce1f895ec88df7a11e523e915

Automatic review by SymPy Bot.

@asmeurer
Member

You can ignore that test failure. There aren't any in Python 2 either, but there seems to be some problem with the bot. It just hangs with

============================== txt doctests start ==============================

bot-tmpjf8lh2/sympy/doc/src/gotchas.txt [100]                               [OK]
bot-tmpjf8lh2/sympy/doc/src/guide.txt [46]                                  [OK]
bot-tmpjf8lh2/sympy/doc/src/install.txt [10]                                [OK]
*** DocTestRunner.merge: 'index.txt' in both testers; summing outcomes.
*** DocTestRunner.merge: 'index.txt' in both testers; summing outcomes.
bot-tmpjf8lh2/sympy/doc/src/modules/assumptions/index.txt [80]              [OK]
*** DocTestRunner.merge: 'index.txt' in both testers; summing outcomes.
bot-tmpjf8lh2/sympy/doc/src/modules/concrete.txt [22]                       [OK]
bot-tmpjf8lh2/sympy/doc/src/modules/core.txt [13]                           [OK]
bot-tmpjf8lh2/sympy/doc/src/modules/evalf.txt [69]                          [OK]
bot-tmpjf8lh2/sympy/doc/src/modules/functions/elementary.txt [24]           [OK]
*** DocTestRunner.merge: 'index.txt' in both testers; summing outcomes.
*** DocTestRunner.merge: 'index.txt' in both testers; summing outcomes.
*** DocTestRunner.merge: 'latex_ex.txt' in both testers; summing outcomes.
bot-tmpjf8lh2/sympy/doc/src/modules/geometry.txt [58]                       [OK]
*** DocTestRunner.merge: 'index.txt' in both testers; summing outcomes.
bot-tmpjf8lh2/sympy/doc/src/modules/integrals/integrals.txt [7]             [OK]
bot-tmpjf8lh2/sympy/doc/src/modules/logic.txt [15]                          [OK]
bot-tmpjf8lh2/sympy/doc/src/modules/matrices.txt [99]                       [OK]
*** DocTestRunner.merge: 'ntheory.txt' in both testers; summing outcomes.
*** DocTestRunner.merge: 'index.txt' in both testers; summing outcomes.
*** DocTestRunner.merge: 'matrices.txt' in both testers; summing outcomes.
bot-tmpjf8lh2/sympy/doc/src/modules/physics/mechanics/advanced.txt [24]     [OK]
bot-tmpjf8lh2/sympy/doc/src/modules/physics/mechanics/examples.txt [183]    [OK]
*** DocTestRunner.merge: 'index.txt' in both testers; summing outcomes.
bot-tmpjf8lh2/sympy/doc/src/modules/physics/mechanics/kane.txt [46]         [OK]
bot-tmpjf8lh2/sympy/doc/src/modules/physics/mechanics/kinematics.txt [62]   [OK]
bot-tmpjf8lh2/sympy/doc/src/modules/physics/mechanics/masses.txt [23]       [OK]
bot-tmpjf8lh2/sympy/doc/src/modules/physics/mechanics/vectors.txt [58]      [OK]
*** DocTestRunner.merge: 'index.txt' in both testers; summing outcomes.
*** DocTestRunner.merge: 'index.txt' in both testers; summing outcomes.
bot-tmpjf8lh2/sympy/doc/src/modules/physics/units.txt [9]                   [OK]
bot-tmpjf8lh2/sympy/doc/src/modules/polys/basics.txt [54]                   [OK]
*** DocTestRunner.merge: 'index.txt' in both testers; summing outcomes.
*** DocTestRunner.merge: 'reference.txt' in both testers; summing outcomes.
bot-tmpjf8lh2/sympy/doc/src/modules/polys/wester.txt [59]                   [OK]
bot-tmpjf8lh2/sympy/doc/src/modules/printing.txt [30]                       [OK]
bot-tmpjf8lh2/sympy/doc/src/modules/rewriting.txt [15]                      [OK]
bot-tmpjf8lh2/sympy/doc/src/modules/series.txt [7]                          [OK]
bot-tmpjf8lh2/sympy/doc/src/modules/solvers/solvers.txt [4]                 [OK]
bot-tmpjf8lh2/sympy/doc/src/modules/statistics.txt [25]                     [OK]
*** DocTestRunner.merge: 'index.txt' in both testers; summing outcomes.
bot-tmpjf8lh2/sympy/doc/src/modules/utilities/autowrap.txt [8]              [OK]
bot-tmpjf8lh2/sympy/doc/src/modules/utilities/codegen.txt [1]               [OK]
*** DocTestRunner.merge: 'index.txt' in both testers; summing outcomes.
bot-tmpjf8lh2/sympy/doc/src/modules/utilities/iterables.txt [5]             [OK]
bot-tmpjf8lh2/sympy/doc/src/tutorial.ru.txt [189]                           [OK]
bot-tmpjf8lh2/sympy/doc/src/tutorial.txt [195]                              [OK]

Does anyone else get this?

@asmeurer
Member

That's in Python 2.5 by the way. That might be important.

@asmeurer asmeurer and 1 other commented on an outdated diff Jan 25, 2012
sympy/functions/elementary/piecewise.py
- for expr, condition in self.args:
- args.append((expr, condition))
- return tuple(args)
+ oth = [ str(o) for o in oth ]
+ raise ValueError("Only one otherwise statement can be specified, got: %s" % ', '.join(oth))
+ new_args = ecs + [oth]
+
+ # evaluation
+ if not evaluate and any([ isinstance(expr, Piecewise) or isinstance(cond, bool) for (expr, cond) in ecs ]):
+ evaluate = True
+ if evaluate:
+ evaluated = cls.eval(*new_args)
+ if evaluated is not None:
+ return evaluated
+ # check exprs and otherwise are Exprs
+ # TODO: UndefinedFunction neither Basic not Expr
@asmeurer
asmeurer Jan 25, 2012 Member

How is this an issue? That would be like having Piecewise((sin, x> 0)) instead of Piecewise((sin(x), x > 0)).

@flacjacket
flacjacket Jan 25, 2012 Member

I'm not sure about sin, but if you call there is a test that uses f = Function('f') as an expression and it raises the error because f is not an Expr. I just realized I could add another or for this, like with GeometryEntity (which subclasses tuple).

@asmeurer
asmeurer Jan 25, 2012 Member

Well, Function('f') is the same as sin. It doesn't make sense. That test should be removed.

Regarding geometry, I don't know if it makes sense to put those in a piecewise. I wouldn't worry about it. We will hopefully rewrite that to be Basic soon anyway.

@flacjacket
flacjacket Jan 25, 2012 Member

Alright, I'll change the test. The geometry stuff comes in when you look at a point inside a shape with .arbitrary_point().

@asmeurer
Member

It appears that the evaluate keyword does not work at all. I guess this would actually rely on http://code.google.com/p/sympy/issues/detail?id=2531 to do correctly. Perhaps a new issue could be opened.

@asmeurer
Member

Well, I didn't necessarily want all the commits squashed into one. I was mostly concerned with the one letter commit messages :)

But don't worry, it's fine. Please do mention in the message what you did to fix the leading_term issue, though.

@flacjacket
Member

Yes, in any instance where there is something you could keep unevaluated, it is forced. For now, that could be changed for nested Piecewise statements, but not evaluating with bools would require that issue.

@asmeurer
Member

I like how nan is automatically included (but not printed). That makes things very clear.

@asmeurer asmeurer commented on an outdated diff Jan 25, 2012
sympy/functions/elementary/piecewise.py
return None
- def doit(self, **hints):
- """
- Evaluate this piecewise function.
- """
- newargs = []
- for e, c in self.args:
- if hints.get('deep', True):
- if isinstance(e, Basic):
- e = e.doit(**hints)
- if isinstance(c, Basic):
- c = c.doit(**hints)
- newargs.append((e, c))
- return Piecewise(*newargs)
+ @property
+ def exprcondpairs(self):
@asmeurer
asmeurer Jan 25, 2012 Member

Add a docstring with doctest for this.

@asmeurer asmeurer commented on an outdated diff Jan 25, 2012
sympy/functions/elementary/piecewise.py
- """
- newargs = []
- for e, c in self.args:
- if hints.get('deep', True):
- if isinstance(e, Basic):
- e = e.doit(**hints)
- if isinstance(c, Basic):
- c = c.doit(**hints)
- newargs.append((e, c))
- return Piecewise(*newargs)
+ @property
+ def exprcondpairs(self):
+ return self.args[:-1]
+
+ @property
+ def otherwise(self):
@asmeurer
asmeurer Jan 25, 2012 Member

Add a docstring with doctest for this.

@flacjacket
Member

SymPy Bot Summary: All tests have passed.

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sY0_kJDA

Interpreter: /opt/python-2.5.6-32bit/bin/python (2.5.6-final-0)
Architecture: Linux (32-bit)
Cache: yes
Test command: setup.py test
master hash: 9258bb9
branch hash: e82666140d59c8908b7e2724a48989e7ce1bd92f

Automatic review by SymPy Bot.

@asmeurer
Member

Have all the things we discussed been well tested (in particular the subtitles with the syntax change)?

@asmeurer
Member

My final comment (for now at least) is to make it clear in the docstring that the old way is deprecated.

Speaking of which, what do you think would be a good deprecation cycle length for this?

@flacjacket
Member

So the bot hung for a while at that point, but it eventually went through.

I think the new syntax is tested pretty well. I'll double check that. Some of the new stuff isn't tested the best, like the .exprcondpairs, .otherwise and the as_leading_term, I'll update when I check that.

Those 1 letter commits were temporary as I made some changes. I realized that I was undoing some of the stuff I did with the first commit, so when I got it back to a working state, I figured I'd just squash it all back down.

@flacjacket
Member

I think everything discussed so far have been addressed and all the changes tested.

I've also opened http://code.google.com/p/sympy/issues/detail?id=3025 for the evaluate keyword with bool conditions.

As for the deprecation cycle, I don't think it needs to be too long, since the old syntax, while not very intuitive or clear, is still technically valid and, by default, will evaluate to the desired expression. It should probably stay in if there is a quick point release after 0.7.2, but a full cycle on the order of 0.7.1 to 0.7.2 (coming up on 6 months now) should be fine.

@asmeurer
Member

That's why I was wondering about the length of the cycle. When we remove the old syntax, any expression that was using it will start to evaluate incorrectly. I think we should discuss deprecation cycles in general on the mailing list.

@asmeurer
Member

OK, look at this:

In [1]: Piecewise((1, x < 0), (2, 1 > 0), (3, True))
/Users/aaronmeurer/Documents/Python/sympy/sympy/sympy/functions/elementary/piecewise.py:83: SymPyDeprecationWarning: 'Pass "otherwise" expression as parameter rather than (expr, True). Adding an otherwise parameter, such as the default NaN, silences this warning.'
  SymPyDeprecationWarning)
Out[1]: 
⎧1  for x < 0
⎨            
⎩2  otherwise

In [2]: Piecewise((1, x < 0), (2, 1 > 0), (3, True), nan)
Out[2]: 
⎧1  for x < 0
⎨            
⎩2  otherwise

I think these are wrong. They should both evaluate to 2. The first should still give the deprecation warning, though.

@asmeurer
Member

I send a message to the mailing list about deprecation cycles in general. That doesn't block this pull request (though it may block the release), so let's move that discussion there.

@flacjacket
Member

Oh, I think I see where I may have been misunderstanding you, I've been thinking about evaluation as more of a simplification (which is what it was before), but I think you're saying evaluation should return the expression of the first True statement, regardless of whether previous expressions were undetermined. That definitely makes more sense, now that I see what you're saying.

@flacjacket
Member

This last 2 commits ('Change Piecewise evaluation behavior' and 'Add Piecewise .doit() to perform evaluation') change the evaluation behavior to what I think you have in mind. The commit messages and docstrings state what I changed around.

@flacjacket
Member

I realized that the evaluation change broke some tests, so I traced the failures as much as I could and applied some heavy-handed changes to get the tests to pass. Namely sympy/integrals/transforms.py L258 and sympy/integrals/meijerint.py L600. Now that I see it, it might be better for the 'doit()' option to stop Piecewise evaluation be the 'piecewise' keyword, rather than 'evaluate', allowing the meijerint.py change to be a bit more precise. I'll see if I can't trace down the Piecewise's that need evaluation in transforms.py, I know they look like Piecewise(otherwise), and get them to evaluate without breaking anything. Tho, at the very least, now all tests pass and no tests raise the deprecation warning.

@flacjacket
Member

I have now changed the keyword for toggling doit() evaluation from 'evaluate' to 'piecewise'. Also, giving only an otherwise expression acts like putting bools in the conditions, in that it will force evaluation unless evaluate is explicitly False. I squashed these changes into the 'Fix other code...' commit, so the line numbers referenced in my previous comment no longer make sense.

@asmeurer
Member

SymPy Bot Summary: There were test failures.

@flacjacket: Please fix the test failures.

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sYvqgKDA

Interpreter: /usr/local/bin/python2.5 (2.5.0-final-0)
Architecture: Darwin (64-bit)
Cache: yes
Test command: setup.py test
master hash: df02ffc
branch hash: 1554502ebdcd642f60a6df92b14a654855e41a7d

Automatic review by SymPy Bot.

@asmeurer
Member
asmeurer commented Feb 1, 2012

SymPy Bot Summary: There were test failures.

@flacjacket: Please fix the test failures.

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sY9YULDA

Interpreter: /Library/Frameworks/Python.framework/Versions/3.2/bin/python3 (3.2.2-final-0)
Architecture: Darwin (64-bit)
Cache: yes
Test command: setup.py test
master hash: 9d3a94e
branch hash: 1554502ebdcd642f60a6df92b14a654855e41a7d

Automatic review by SymPy Bot.

@asmeurer
Member
asmeurer commented Feb 5, 2012

Should we do something if more than one True condition is found (with new syntax)? For example, Piecewise((x, True), (y, True), nan).

@asmeurer asmeurer and 1 other commented on an outdated diff Feb 5, 2012
sympy/functions/elementary/piecewise.py
- @property
- def free_symbols(self):
- """
- Return the free symbols of this pair.
- """
- # Overload Basic.free_symbols because self.args[1] may contain non-Basic
- result = self.expr.free_symbols
- if hasattr(self.cond, 'free_symbols'):
- result |= self.cond.free_symbols
- return result
+ Piecewise can also evaluate the function, which is set by the ``evaluate``
+ keyword, that is disabled by default. Having explicitly True or False
+ conditions will enable evaluation unless the ``evaluate`` option is
+ explicitly False. When evaluate is True, it returns the first expression
@asmeurer
asmeurer Feb 5, 2012 Member

This is not entirely true, since we can't represent the True conditions (issue 2531). Perhaps a note that that can't be done yet should be added here.

@asmeurer
asmeurer Feb 5, 2012 Member

I'm also not sure if this is right:

In [9]: Piecewise((x, True), (y, x < 1), nan, evaluate=False)
Out[9]: {x  otherwise
@asmeurer
asmeurer Feb 5, 2012 Member

If you're up to it, you could just implement issue 2531, and this would be less messy :)

@flacjacket
flacjacket Feb 5, 2012 Member

I can take a crack at it. If I can't get something working nicely, the docstring should need to be changed to address this limitation.

@flacjacket
Member

I don't think we should necessarily disallow having multiple cases be True, a lazy set of conditions, such as x>1 then x>0, should be valid, and at least Mathematica doesn't complain at it. It might be a good idea to raise a warning on something like that so it can easily be identified as a problem if that's not intended.

@asmeurer
Member
asmeurer commented Feb 8, 2012

You're right. I didn't consider that this could come from having redundant conditions. In general, it's very difficult to tell if conditions overlap with each other, and anyway, that sort of thing should not happen in the constructor (rather it should happen in some simplification function). And sometimes it's useful to define redundant conditions (where the expr is actually the same in the overlap).

So I guess we should just choose the first one (and document it).

@flacjacket
Member

So with the new Basic Booleans, there is the question of how much to use them as return values instead of bools. Some things, such as .contains() in core.sets, uses Boolean functions, and so would return a Boolean value, but most properties will still return a bool. This could be a problem if people specifically check if something is a bool (we do 33 times in master, grep -P 'isinstance\(.*bool\)' sympy -R). This shouldn't be too much of a problem if we just sympify things before doing the isinstance, but it could definitely be something that is easily missed and silently break something.

@asmeurer
Member
asmeurer commented Feb 9, 2012

Not just isinstance(bool), but also any is {True,False,None} check would be broken.

@asmeurer
Member
asmeurer commented Feb 9, 2012

I guess we can use the rule of thumb that things relating the the assumptions system (i.e., using three valued logic) should return the Basic types, and everything else can stick with the Python types. It's still a fuzzy distinction, but hopefully the majority of the time, it shouldn't matter which type is returned.

@flacjacket
Member

So looking at what I have fixed so far and what is still failing, switching the assumptions and boolalg functions to use the Basic objects breaks a ton of stuff, which I'm not sure I can get to in a very timely way with school currently in full swing. If you want this pushed, I can fix up the documentation to make it very explicit what happens with boolean conds, and cherry pick the other commit to a new branch, otherwise, this pull may linger for a while.

@asmeurer
Member

Yes, just do that. The most important thing is to get Piecewise to work.

@flacjacket
Member

Alright, I pulled the Basic bools stuff into a new branch and did a rewrite of the documentation. Let me know what you think of this.

@rlamy
Member
rlamy commented Feb 17, 2012

I'd like to take care of the Boolean stuff. I'm working on a branch (based on master), I hope I can get it ready soon, and that merging with this won't be too painful.

@flacjacket
Member

@rlamy That'd be great, you have a much deeper understanding of the logic functionality something like that would touch.

@asmeurer
Member

So should this branch wait for yours, or can we go ahead without it?

@rlamy
Member
rlamy commented Feb 22, 2012

It might take a while, so if this can work without the new Booleans, I guess it's better not to wait.

@asmeurer
Member

This needs to be rebased.

So I guess without the Basic booleans, we should just raise NotImplementedError when we would need them (and maybe leave in the stub code so that it's easy to make it work when we have them).

Alternately, we could just put in regular booleans and XFAIL the test in test_args. It would give more functionality, but the objects would break with many things.

@flacjacket
Member

I have rebased and added a commit to cause the boolean conds with evaluate=False to throw an error. There is one part of meijerint that would need this to work to have the desired effect, so that is changed to be careful the args it passes to the Piecewise function. There were also a couple tests in piecewise that needed to move to an XFAIL.

@rlamy rlamy commented on an outdated diff Feb 24, 2012
sympy/functions/elementary/piecewise.py
+ # c = BasicNone
+ # new_ecs.append(Tuple(e,c))
+ #ecs = new_ecs
+ else:
+ evaluate = True
+ # Having no expr/cond pairs should also trigger evaluation unless explicitly False
+ if len(ecs) == 0 and evaluate is not False:
+ return oth
+
+ new_args = ecs + [oth]
+ if evaluate:
+ return cls.eval(*new_args)
+
+ # check exprs and otherwise are Exprs
+ from sympy.geometry.entity import GeometryEntity
+ if not all([ isinstance(expr, Expr) or isinstance(expr, GeometryEntity) for (expr, _) in ecs ]):
@rlamy
rlamy Feb 24, 2012 Member

That kind of static type-checking is more likely to be a PITA in the future than a help. I think it's best not to bother.

@rlamy rlamy commented on an outdated diff Feb 24, 2012
sympy/functions/elementary/piecewise.py
- newargs = []
- for ec in args:
- pair = ExprCondPair(*ec)
- cond_type = type(pair.cond)
- if not (cond_type is bool or issubclass(cond_type, Relational) or \
- issubclass(cond_type, Number) or \
- issubclass(cond_type, Set) or issubclass(cond_type, Boolean)):
- raise TypeError(
- "Cond %s is of type %s, but must be a bool," \
- " Relational, Number or Set" % (pair.cond, cond_type))
- newargs.append(pair)
-
- if options.pop('evaluate', True):
- r = cls.eval(*newargs)
+ ecs = [ Tuple(*sympify(arg)) for arg in args if hasattr(arg, '__iter__') ]
+ oth = [ arg for arg in args if not hasattr(arg, '__iter__') ]
@rlamy
rlamy Feb 24, 2012 Member

That's very brittle: what if we want to work with objects that happen to be iterable?

@flacjacket
Member

Thanks @rlamy, I've addressed the couple things you mentioned. I dropped the type checking of the expression part of the expr/cond pairs and the otherwise expression, but left the checking on the conditions, only Relationals or Booleans make sense as conditions. I also changed the expr/cond pairs so they need to have a tuple, Tuple or list as the container, rather than just checking that the args are iterable.

@rlamy
Member
rlamy commented Feb 26, 2012

Well, actually I think it's wrong to use any sort of type-checking to distinguish between the pairs and the "otherwise". It should only be based on position.

More generally, the more I look at the problem, the more I feel that the change in syntax only brings complications for little benefit. To introduce the new Booleans, I would actually prefer to only fix what's broken in Piecewise (ExprCondPair, conds with baroque types) without changing the syntax or adding new behaviour. That would basically mean putting this PR on hold.

@asmeurer
Member

The syntax now is a little more complicated, but that's just because of the backwards compatibility. Once we break that, it will become cleaner in my opinion.

@rlamy
Member
rlamy commented Feb 27, 2012

No, this actually makes the implementation more complicated: currently, something like for expr, cond in self.args is enough to handle all the arguments, but this forces us to deal with the "otherwise" value in addition.

A real simplification would be to replace Piecewise with nested IfElse(condition, true_expr, false_expr) objects.

@asmeurer
Member

I disagree with that. Flat is better than nested.

expr, cond doesn't work because not every expression has a condition.

flacjacket added some commits Jan 15, 2012
@flacjacket flacjacket New Piecewise syntax and fix Piecewise issues
Piecewise syntax is changed to allow for a more sensible definition of
the otherwise condition. Before, an expr/cond pair was specified where
cond was given as True. Now, otherwise can either be passed as the final
argument after a sequence of expr/cond 2-tuples. The old otherwise
syntax using (otherwise, True) raises a deprecation warning.

The ExprCondPair class is removed in favor of Tuples to store the
expr/cond pairs.

The '.as_leading_term()' function is changed so that it takes the sum of
all the expressions and, if it is not NaN, the otherwise statement, and
returns the leading term of the sum. Ideally, '.as_leading_term()' would
return a Piecewise function, but doing this breaks the evaluation of
'abs(x-a).nseries(x, 1)'.

Piecewise no longer accepts an Interval as a condition. To implement the
same functionality, either the '.as_relational()' or '.contains()'
methods of Interval can be used. Piecewise also no longer accepts
numbers as conditions.

The properties '.exprcondpairs' and '.otherwise' are implemented to
easily access the expr/cond pairs and otherwise statement, respectively.

Addresses issues: 2101, 2567, 2626, 2710, 2726
779ef82
@flacjacket flacjacket Additional Piecewise testing
Changes allowing evaluate keyword to override default behavior of
clearing nested Piecewise expressions. More testing of new Piecewise
evaluate keyword behavior, including an XFAIL test related to Issue
3025, where evaluate=False does not override behavior when bools are in
the conditions.

Includes new tests and doctests for '.exprcondpairs' and '.otherwise'
properties.

Re-enables testing for expressions in expr/cond pairs and the otherwise
statement to be Expr or GeometryEntity. GeometryEntity is needed due to
the use of Piecewise in the geometry module for '.arbitrary_point()'.
569cd41
@flacjacket flacjacket Change Piecewise evaluation behavior
Piecewise evaluation now returns the first expression which is
explicitly True. If no conditions are True, the otherwise expression is
returned. Evaluation can be triggered by setting the 'evaluate' keyword
to True. When a boolean is given as a condition, evaluation is enabled
unless specifically disabled by setting 'evaluate' to False. In this
case, any explicitly False conditions are removed and any explicitly
True conditions are given as the otherwise statement. This is done so
bools are kept out of args, see Issue 3025. The collapsing of nested
Piecewise functions is moved from eval to the __new__ method. This is
performed by default, but setting 'evaluate' to False will disable it.
4ba539e
@flacjacket flacjacket Add Piecewise .doit() to perform evaluation
Added a .doit() method to Piecewise functions. This allows for
forced evaluation of the Piecewise function. If you want to call doit()
to evaluate args but not evaluate the Piecewise function itself,
evaluate=False can be added to the hints.
c86dea6
@flacjacket flacjacket Fix other code to work with new Piecewise evaluation
Change '.doit()' keyword for evaluating Piecewise objects from
'evaluate' to 'piecewise' and set other functions to use this to
avoid evaluation of Piecewise when required. Having no expr/cond pairs
is treated like having bools in the conditions.
7fe8654
@flacjacket flacjacket Fix and add tests for Piecewise printing 54367c0
@flacjacket flacjacket Fix up documentation of Piecewise objects
Improve documentation of Piecewise objects, specifying how the new
evaluation functionality works, how it is overridden and the limitations
posed by not being able to have bools in the args.
b1b27fa
@flacjacket flacjacket Piecewise raises error with bool args and evaluate=False
Change behavior of Piecewise so if bools are given as args and it is
given the keyword evaluate=False, it raises a NotImplementedError. This
can be changed once classes implementing boolean logic that subclass
Basic are implemented. Changes are made in meijerint to parse the args
of a Piecewise function used in that module which may have bool
conditions, but should not be evaluated. This can be changed to use
evaluate=False when the Basic boolean classes are implemented.
15e54a0
@flacjacket flacjacket Fix type checking in Piecewise
Change checking in Piecewise to take expr/cond pairs only in tuples,
Tuples or lists, all other expressions go to otherwise. Drop the type
checking for the exprs and otherwise statement.
e8cbd8d
@rlamy rlamy commented on the diff Feb 27, 2012
sympy/functions/elementary/tests/test_piecewise.py
@@ -8,11 +8,26 @@
def test_piecewise():
# Test canonization
- assert Piecewise((x, x < 1), (0, True)) == Piecewise((x, x < 1), (0, True))
- assert Piecewise((x, x < 1), (0, False), (-1, 1>2)) == Piecewise((x, x < 1))
- assert Piecewise((x, True)) == x
- raises(TypeError,"Piecewise(x)")
+ assert Piecewise((x, x > 1)) == Piecewise((x, x > 1), S.NaN)
+ assert Piecewise((x, x > 1), (0, True), S.NaN) == 0
@rlamy
rlamy Feb 27, 2012 Member

Huh?? This should be Piecewise((x, x > 1), 0).

@asmeurer
asmeurer Feb 27, 2012 Member

No, this is right. NaN is the otherwise condition. See the prior discussion, both here and on the mailing list. During the deprecation cycle, you can force (expr, cond) pairs to be viewed as that (rather than (expr_otherwise, True)) by manually adding nan as the otherwise condition (this is the default otherwise condition, so this changes nothing).

@rlamy
rlamy Feb 27, 2012 Member

But the (x, x > 1) part has disappeared! That is the problem, not whether the "otherwise" should be 0 or NaN.

@asmeurer
asmeurer Feb 28, 2012 Member

That's because the piecewise evaluated to 0, which had a True condition.

@rlamy
rlamy Feb 28, 2012 Member

Translated to Python code, Piecewise((x, x>1), (0, True), S.NaN) means

if x > 1:
    return x
elif True:
    return 0
else:
    return S.NaN

which is clearly equivalent to

if x > 1:
    return x
else:
    return 0

but not to simply return 0.

@asmeurer
asmeurer Feb 28, 2012 Member

OK, I see what you're getting at. This is exactly why we need Piecewise(evaluate=False).

@rlamy
rlamy Feb 28, 2012 Member

Let's move the discussion to issue 3025, so that it doesn't disappear when this review is done, OK?

@asmeurer
Member

By the way, I do think that it actually would be useful to have some method of Piecewise that would return some kind of IFElse representation. That is exactly what the lambda printer uses, as I remember, so it would essentially move the logic from there into Piecewise, where it might be useful to others as well. But the .args should be expr, cond pairs with otherwise at the end as discussed in issue 2626.

@rlamy rlamy referenced this pull request Feb 29, 2012
Merged

Refactor ExprCondPair #1095

flacjacket added some commits Mar 16, 2012
@flacjacket flacjacket Merging with master 256094c
@flacjacket flacjacket Merge branch 'master' into piecewise_fixes
Conflicts:
	sympy/functions/elementary/piecewise.py
	sympy/printing/tests/test_str.py
b2bc48b
@flacjacket flacjacket Merge branch 'master' into piecewise_fixes
Conflicts:
	sympy/solvers/tests/test_ode.py
e500e7d
@flacjacket flacjacket Merge branch 'master' into piecewise_fixes
Conflicts:
	sympy/printing/tests/test_str.py
	sympy/stats/crv_types.py
972d8fe
@asmeurer
Member
asmeurer commented Jun 2, 2012

SymPy Bot Summary: ❗️ There were merge conflicts; could not test the branch.

@flacjacket: Please rebase or merge your branch with master. See the report for a list of the merge conflicts.

Test results html report: http://reviews.sympy.org/report/agZzeW1weTNyDAsSBFRhc2sY7N0aDA

Interpreter: /Library/Frameworks/Python.framework/Versions/3.2/bin/python3 (3.2.2-final-0)
Architecture: Darwin (64-bit)
Cache: yes
Test command: ./bin/test && ./bin/doctest
master hash: 53af081
branch hash: 972d8fe

Automatic review by SymPy Bot.

@jrioux
Member
jrioux commented Oct 22, 2012

SymPy Bot Summary: ❗️ There were merge conflicts (could not merge flacjacket/piecewise_fixes (972d8fe) into master (57e94e4)); could not test the branch.
@flacjacket: Please rebase or merge your branch with master. See the report for a list of the merge conflicts.

@matthew-brett matthew-brett pushed a commit to matthew-brett/sympy that referenced this pull request Nov 21, 2014
@mattpap @rlamy mattpap + rlamy roots() won't hang on large examples like in #1009 8c138b0
@skirpichev skirpichev referenced this pull request in diofant/diofant Mar 28, 2016
Open

Drop ExprCondPair class, fix Piecewise syntax #256

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