Fixed pretty printing and codegen error messages for empty piecewise #15010

ramvenkat98 · 2018-08-02T02:09:32Z

New behavior:

Manually constructing Piecewise() now raises a type error
Piecewise evaluated with no matching condition gives nan

References to other Issues or PRs

Fixes #15005 based on the discussion there

Brief description of what is fixed or changed

Other comments

Release Notes

functions
- Piecewise with no arguments not allowed anymore
- Piecewise evaluated with no matching conditions gives nan

1. pprint called on an empty Piecewise function now prints "undefined" 2. codegen print functions raise the relevant "No otherwise statement error" when encountering Piecewise functions

sympy-bot · 2018-08-02T02:09:43Z

✅

Hi, I am the SymPy bot (v127). I'm here to make sure this pull request has a release notes entry. Please read the guide on how to write release notes.

Click here to see the pull request description that was parsed.

<!-- Your title above should be a short description of what
was changed. Do not include the issue number in the title. -->
New behavior:
- Manually constructing Piecewise() now raises a type error
- Piecewise evaluated with no matching condition gives nan
#### References to other Issues or PRs
<!-- If this pull request fixes an issue, write "Fixes #NNNN" in that exact
format, e.g. "Fixes #1234". See
https://github.com/blog/1506-closing-issues-via-pull-requests .-->
Fixes #15005 based on the discussion there
#### Brief description of what is fixed or changed
#### Other comments
#### Release Notes
<!-- Write the release notes for this release below. See
https://github.com/sympy/sympy/wiki/Writing-Release-Notes for more information
on how to write release notes. If there is no release notes entry for this PR,
write "NO ENTRY". The bot will check your release notes automatically to see
if they are formatted correctly. -->
<!-- BEGIN RELEASE NOTES -->
* functions
  * Piecewise with no arguments not allowed anymore
  * Piecewise evaluated with no matching conditions gives nan
<!-- END RELEASE NOTES -->

Your release notes are in good order.

Here is what the release notes will look like:

functions
- Piecewise with no arguments not allowed anymore (#15010 by @ramvenkat98)
- Piecewise evaluated with no matching conditions gives nan (#15010 by @ramvenkat98)

This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.2.1.

Note: This comment will be updated with the latest check if you edit the pull request. You need to reload the page to see it.

Update

The release notes on the wiki have been updated.

cbm755

I'm not sure if this is the fix we want but here are some suggestions to improve it.

Please check if Julia etc changes are really needed

cbm755 · 2018-08-02T03:30:51Z

sympy/printing/julia.py

@@ -418,7 +418,7 @@ def _print_yn(self, expr):


    def _print_Piecewise(self, expr):
-        if expr.args[-1].cond != True:
+        if len(expr.args) == 0 or expr.args[-1].cond != True:


I thought all of these codegen were working correctly before?

I changed this because the codegens were raising a tuple index out of range error later on in the function (when given an empty piecewise function) instead of raising the relevant ValueError that there isn't a default block.

cbm755 · 2018-08-02T03:52:37Z

sympy/printing/pretty/pretty.py

+            D.baseline = D.height()//2
+            D.binding = prettyForm.OPEN
+            return D
+


This needs a test for pprint and latex

Have added the test case for both

Fixed the same bug (reported for pprint) for latex printing too Added test cases for pprint and latex printing as suggested by review

asmeurer · 2018-08-02T05:08:34Z

I didn't know empty Piecewise was allowed. What does it represent? Is it actually used somewhere?

I'm not a fan of "undefined" for the pretty printing. I think just printing it as Piecewise() is clearer.

asmeurer · 2018-08-02T05:10:19Z

This could use a release notes entry.

asmeurer · 2018-08-02T05:11:00Z

As noted on the issue, I think the empty Piecewise() should just raise an exception, or possibly give nan. @smichr what do you think?

…t and latex" This reverts commit ebcd67b.

…ecewise" This reverts commit 719af6f.

…ives nan Manually constructed Piecewise raises TypeError Piecewise when evaluated (subs or evalf) without matching condition gives nan A test was over-written to support this

ramvenkat98 · 2018-08-06T23:59:11Z

@asmeurer @cbm755 @smichr

Hi,

I've made changes according to the discussion in the issue thread #15005. Currently, Travis is failing to build on exactly one test and the error doesn't seem to be related to the code - could you restart it? Also, please let me know if you think this fix is ok.

Thanks

asmeurer · 2018-08-07T01:22:25Z

sympy/functions/elementary/tests/test_piecewise.py

@@ -84,11 +85,16 @@ def test_piecewise():
        ).subs(x, 1) == Piecewise((-1, y < 1), (2, True))
    assert Piecewise((1, Eq(x**2, -1)), (2, x < 0)).subs(x, I) == 1

+    p6 = Piecewise((x, x > 0))
+    n = symbols('n', negative = True)
+    assert p6.subs(x, n) == Undefined


What is "Undefined"?

@asmeurer

"Undefined" was defined as S.NaN in sympy/functions/elementary/tests/piecewise.py and used in the piecewise integration code, and the tests for piecewise integration so Undefined == nan (I used that to stay consistent with the piecewise integration code but could use S.NaN if that's clearer)

cbm755

Note the class docs say:

         - If the function is evaluated at a place where all conditions are False,
          a ValueError exception will be raised.

which isn't true after this change (?)

cbm755 · 2018-08-07T05:05:00Z

sympy/functions/elementary/tests/test_piecewise.py

@@ -84,11 +85,16 @@ def test_piecewise():
        ).subs(x, 1) == Piecewise((-1, y < 1), (2, True))
    assert Piecewise((1, Eq(x**2, -1)), (2, x < 0)).subs(x, I) == 1

+    p6 = Piecewise((x, x > 0))
+    n = symbols('n', negative = True)


negative=True (when used in kwargs)

cbm755 · 2018-08-07T05:19:44Z

sympy/functions/elementary/tests/test_piecewise.py

    # False condition is never retained
-    assert Piecewise((x, False)) == Piecewise(
-        (x, False), evaluate=False) == Piecewise()
+    raises (TypeError, lambda: Piecewise((x, False)))


this is a lossy change. Add your new test but modify the existing one as:

assert Piecewise((2*x, x < 0), (x, False)) == Piecewise((2*x, x < 0), (x, False), evaluate=False) == Piecewise((2*x, x < 0))

cbm755 · 2018-08-07T05:20:21Z

sympy/functions/elementary/tests/test_piecewise.py

@@ -30,9 +30,10 @@ def test_piecewise():
        Piecewise((x, Or(x < 1, x < 2)), (0, True))
    assert Piecewise((x, x < 1), (x, x < 2), (x, True)) == x
    assert Piecewise((x, True)) == x
+    #Empty Piecewise not accepted


# Empty... (with space)

cbm755 · 2018-08-07T05:33:52Z

sympy/functions/elementary/piecewise.py

                e = e._subs(old, new)
            args[i] = (e, c)
            if c == True:
                break
+        if not args_exist:
+            return Undefined


I haven't learning about _eval_subs but is it ok to return something other than self.func... here? Is it safer to do if not args_exist: args = ((Undefined, True))?

Also, is this the best place to make this change? Is this the only code path that looks like evaluate/simplify/subs/canonicalize/etc where we want nan instead of exception?

This code starts with # this is strictly not necessary, and yet now it will be necessary.

(Note this is not a well-informed opinion: if you've thought about this and this is the right place, then say so.)

Fixed issues based on the reviews given

ramvenkat98 · 2018-08-07T06:42:10Z

@cbm755

I think the modified function (_eval_subs) was already necessary. If I try to bypass this function by defining it as def _eval_subs(self, old, new): return self.func(*self.args), it will not substitute values correctly. Instead, it would give for example:

>>> f = Piecewise((x, x>0), (x, x<-1))
>>> f.subs(x,0.5)
Piecewise((x, (x > 0) | (x < -1)))

I think the comment might mean that the function could have been written more compactly like so:

def _eval_subs(self, old, new):
        args = list(self.args)
        for i, (e, c) in enumerate(args):
            c = c._subs(old, new)
            e = e._subs(old, new)
            args[i] = (e,c)
        return self.func(*args)

which seems to give the correct result as expected. Instead, it is written the way it is to avoid evluating e._subs when c is known to be False and to avoid looking at the remaining (e,c) values when we've already found one c value that evaluates to True. I think this part of the function is not strictly necessarily, but makes it more efficient.

So I think this is a good place to make the change, because it is always passed through when calling the subs/evaluate methods and the function overall is already necessary.

About returning Undefined at the end, I looked through the code and I don't see an instance where returning Undefined straightaway would be a problem, but I'm not sure either so I agree that it is safer to do if not args_exist: args = ((Undefined, True),) there and have made that change.

Relating to what the class docs said, I'm not sure if they were true originally either because the original behavior seems to be that subs/evaluate returns Piecewise() when evaluated at a point where all conditions are False. In test_piecewise, I can only find one instance of a ValueError and that happens after it is rewritten as an ITE. I updated the class docs to reflect that it will instead return nan in such cases.

I've also updated the test cases and the syntax issues pointed out.

Thanks

ramvenkat98 · 2018-08-08T03:51:30Z

@asmeurer @cbm755 @smichr

I've made the changes requested in the review, are there any other changes that should be made or can this pull request be merged?

asmeurer · 2018-08-09T18:45:23Z

sympy/functions/elementary/tests/test_piecewise.py

+    assert Piecewise((2*x, x < 0), (x, False)) == \
+        Piecewise((2*x, x < 0), (x, False), evaluate=False) == \
+        Piecewise((2*x, x < 0))
+    raises (TypeError, lambda: Piecewise((x, False)))


I'm not sure about this one. I think it would be better for this case to also return nan, and only do TypeError for explicit Piecewise() with no arguments. It's possible for piecewise like these to be constructed, for instance, if a condition evaluates to False. Plus the error message would be confusing in this case.

… error Piecewise with arguments that are all False now return nan, not raise TypeError

ramvenkat98 · 2018-08-09T22:55:10Z

@asmeurer

I've changed the code so that only explicitly constructed empty piecewise functions raise a TypeError.

So the new behavior is as follows:

>>> Piecewise()
TypeError: At least one (expr, cond) pair expected.
>>> Piecewise((x, False))
nan

Is this ok?

Thanks

asmeurer · 2018-08-09T23:00:37Z

Yes, that behavior is more reasonable to me.

cbm755 · 2018-08-10T04:23:32Z

Sorry for "one more thing" but I get the impression if len(meh) == 0: isn't very Pythonic; in Python3 various things that I think of as lists get angry if you ask them how long they are. Should we be worried about that here?

Piecewise(<some generator>) will fail with this change? Is if args: the "right thing"? Should we merge this an file a new bug (and a new test)?

ramvenkat98 · 2018-08-10T05:46:04Z

@cbm755

I think here it could still work for generators because args would be a tuple even if a generator argument was passed in (since the function argument includes *args, which would automatically package the arguments into the new tuple args). I tested this out in the following cases, where it works:

>>> from sympy import *; from sympy.abc import *
>>> def gen_f(): yield x; yield x > 0
... 
>>> Piecewise(gen_f())
Piecewise((x, x > 0))
>>> Piecewise(gen_f()).subs(x, 5)
5
>>> Piecewise(gen_f()).subs(x, -1)
nan
>>> def gen_f2(): yield (x, x>0); yield (x+1, x<=0)
... 
>>> Piecewise(*gen_f2())
Piecewise((x, x > 0), (x + 1, True))

I'm not sure if there is a case involving a generator that would break it, but I don't think so because I think *args always implies that args is a tuple.

cbm755 · 2018-08-10T06:23:00Z

ok, thanks for checking!

Will merge in 24 hours.

ramvenkat98 · 2018-08-12T05:34:57Z

@cbm755 @asmeurer

Could this be merged? Thanks

Fixed pretty printing and codegen error messages for empty piecewise

719af6f

1. pprint called on an empty Piecewise function now prints "undefined" 2. codegen print functions raise the relevant "No otherwise statement error" when encountering Piecewise functions

cbm755 requested changes Aug 2, 2018

View reviewed changes

Fixed same bug for latex printing, added test cases for pprint and latex

ebcd67b

Fixed the same bug (reported for pprint) for latex printing too Added test cases for pprint and latex printing as suggested by review

ramvenkat98 added 3 commits August 6, 2018 18:55

Revert "Fixed same bug for latex printing, added test cases for pprin…

c427f04

…t and latex" This reverts commit ebcd67b.

Revert "Fixed pretty printing and codegen error messages for empty pi…

0fde95b

…ecewise" This reverts commit 719af6f.

Piecewise() raises TypeError, Piecewise without matching conditions g…

230d32a

…ives nan Manually constructed Piecewise raises TypeError Piecewise when evaluated (subs or evalf) without matching condition gives nan A test was over-written to support this

asmeurer reviewed Aug 7, 2018

View reviewed changes

cbm755 reviewed Aug 7, 2018

View reviewed changes

Fixed issues based on reviews

d85c9cb

Fixed issues based on the reviews given

asmeurer reviewed Aug 9, 2018

View reviewed changes

Abdullahjavednesar added the PR: author's turn The PR has been reviewed and the author needs to submit more changes. label Aug 9, 2018

Updated code based on review - only explicitly empty Piecewise raises…

5273e8b

… error Piecewise with arguments that are all False now return nan, not raise TypeError

asmeurer approved these changes Aug 9, 2018

View reviewed changes

cbm755 approved these changes Aug 10, 2018

View reviewed changes

cbm755 merged commit e77a128 into sympy:master Aug 12, 2018

cbm755 removed the PR: author's turn The PR has been reviewed and the author needs to submit more changes. label Aug 12, 2018

ramvenkat98 deleted the pprint_empty_piecewise_15005 branch August 13, 2018 17:46

Uh oh!

Fixed pretty printing and codegen error messages for empty piecewise #15010

Fixed pretty printing and codegen error messages for empty piecewise #15010

Uh oh!

Conversation

ramvenkat98 commented Aug 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

References to other Issues or PRs

Brief description of what is fixed or changed

Other comments

Release Notes

Uh oh!

sympy-bot commented Aug 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cbm755 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ramvenkat98 Aug 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asmeurer commented Aug 2, 2018

Uh oh!

asmeurer commented Aug 2, 2018

Uh oh!

asmeurer commented Aug 2, 2018

Uh oh!

ramvenkat98 commented Aug 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ramvenkat98 Aug 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbm755 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ramvenkat98 commented Aug 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ramvenkat98 commented Aug 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ramvenkat98 commented Aug 9, 2018

Uh oh!

asmeurer commented Aug 9, 2018

Uh oh!

cbm755 commented Aug 10, 2018

Uh oh!

ramvenkat98 commented Aug 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cbm755 commented Aug 10, 2018

Uh oh!

ramvenkat98 commented Aug 12, 2018

Uh oh!

Uh oh!

ramvenkat98 commented Aug 2, 2018 •

edited

Loading

sympy-bot commented Aug 2, 2018 •

edited

Loading

cbm755 left a comment •

edited

Loading

ramvenkat98 Aug 2, 2018 •

edited

Loading

ramvenkat98 Aug 7, 2018 •

edited

Loading

ramvenkat98 commented Aug 7, 2018 •

edited

Loading

ramvenkat98 commented Aug 8, 2018 •

edited

Loading

ramvenkat98 commented Aug 10, 2018 •

edited

Loading