OK, so with Ondrej's help, we decided that the right answer is to use -((t+n*t)/n). Everything seems to be working now.
On Mon, Jun 29, 2009 at 12:15 AM, Aaron S. Meurer <[email protected]>wrote: > I think you could use option 1 as a work around. As I mentioned before, > the bug here isn't in simplify(), but in Mul, which doesn't properly > distribute the minus sign in (t+n*t)/-n giving:>>> print (t+n*t)/-n > -(t + n*t)/n > >>> print -(t+n*t)/n > (-t - n*t)/n > >>> (t+n*t)/-n == -(t+n*t)/n > False > > As far as fixing this, I was able to come up with a patch that fixes the > issue, but it breaks some things, like product(). I created issue 1497 with > the basic patch, if you are interested in looking into it (I don't have time > to do it myself right now). If it turns out that changing this behavior > breaks too much of SymPy, then you need to fix whatever function you are > using so that it returns (-t-n*t)/n instead of -(t+n*t)/n. > > If you are worried that simplify might do something different in the future > (a valid concern), do expand(left_side) == expand(right_side). > expand() will expand the fraction, and it should always do the same thing. > Use expand_mul() if you are super worried (which you shouldn't be. If > someone changes expand *that* much, they should be willing to change some > tests :) ). > >>> print expand((t+n*t)/-n) > -t - t/n > >>> print expand(-(t+n*t)/n) > -t - t/n > > Aaron Meurer > On Jun 28, 2009, at 9:51 PM, Ryan Krauss wrote: > > So, I have 3 ideas for how to fix the test in test_solvers.py that my > simplify_minus_sign branch breaks. The problem is that the expected output > is > z = -(t+n*t)/n > but simply typing that expression on the right hand side of an assertion > causes it to be evaluated as > (-t-n*t)/n > so that the assertion fails. I think the core issue is that the results > from solve_linear_system are passed through simplify. > > Here are the three options that I see: > > 1. use simplify on the right hand side of the assertion: > assert (test_result ) == simplify(-(t+n*t)/n) > > This is dangerous in that simplify is being used on the right and left hand > sides of the assertion. A bug in simplify could alter both sides of the > assertion and the test could still pass. This may be fine, since this test > isn't supposed to test simplify. As long as simplify has its own long list > of tests that catch all possible bugs, everything is fine. But this > actually wasn't the case and a test failure outside of test_simplify.py > revealed a bug in my initial patch. > > 2. use -1*expr.extract_multiplicatively(-1) on the right hand size of the > assertion. > > This is basically doing the simplification work by hand. Some of the same > arguements could apply as in #1, but maybe extract_multiplicatively isn't as > complicated as simplify and there is less risk. > > 3. use a string in the assertion: str(test_result) == '-(t + n*t)/n' > > The issue here is that a change to the printer could cause the test to fail > even if there is nothing really wrong with the output (whitespace > differences for instance). > > Which of these 3 is best or should another option be pursued? > > Thanks, > > Ryan > > On Sat, Jun 27, 2009 at 4:41 PM, Ryan Krauss <[email protected]> wrote: > >> My assumption is that the output from solve_linear_system gets passed >> through simplify. Otherwise my changes to simplify wouldn't have broken >> this test. That also explains why res[z] == simplify(my_res) is True (I >> think). >> >> So, while I agree that we should use sympy expressions in the tests >> whereever possible, I don't know how to create a sympy expression for >> >> z = -(t+n*t)/n >> >> without using simplify. Using simplify on the expected result of the test >> seems dangerous because someone like me modifying simplify could break >> something and still pass tests. >> >> >> On Sat, Jun 27, 2009 at 4:12 PM, Aaron S. Meurer <[email protected]>wrote: >> >>> Mul automatically distributes Number coefficients, including -1. The >>> question isn't how to make my_res not do it, but why res[z] doesn't >>> automatically do it. Did you do something like this? >>> print -(x+y)/z >>> (-x - y)/z >>> >>> print (x+y)/-z >>> -(x + y)/z >>> >>> This looks like it is a bug in Mul that should be fixed. >>> >>> I don't think you should test strings unless there is a specific reason >>> that you cannot test sympy expressions, such as testing a printer or an >>> expression with dummy variables in it. Consider this: if in the future, >>> automatic combining changes somehow, then a test sympy_func == sympy_expr >>> will still pass, because both sides will automatically combined in the new >>> way the same (assuming the new combining doesn't break sympy_func. On the >>> other hand, if the test was structured as str(sympy_func) == "sympy_expr", >>> then whoever is trying to write the new automatic combining has to go in and >>> manually fix the test). I just spent three weeks changing the automatic >>> combining of exponents in Mul so that they don't always combine, and I can't >>> imagine how much harder my life would have been if I had to go in and >>> rewrite a ton of tests because they were based on strings of expressions, >>> not expressions themselves. >>> >>> Aaron Meurer >>> On Jun 27, 2009, at 2:54 PM, Ryan Krauss wrote: >>> >>> Is it a good or a bad idea to re-write the test like this: >>> In [45]: str(res[z]) == '-(t + n*t)/n' >>> Out[45]: True >>> >>> >>> On Sat, Jun 27, 2009 at 3:17 PM, Ryan Krauss <[email protected]>wrote: >>> >>>> My patch to simplify a minus 1 from the numerator and denominator of a >>>> fraction causes some failures in test_solvers because the results now have >>>> a >>>> minus 1 factored out. I tried changing one of the tests so that the result >>>> would be the expected one and ran into an interesting problem. The whole >>>> session is below, but here is the core issue: >>>> >>>> In [6]: res[z] >>>> Out[6]: >>>> -(t + n⋅t) >>>> ────────── >>>> n >>>> >>>> In [7]: res[z] == -(t+n*t)/n >>>> Out[7]: False >>>> >>>> The problem is that the expression on the right is apparently altered as >>>> it is executed so that >>>> In [2]: my_res = -(t+n*t)/n >>>> >>>> In [3]: my_res >>>> Out[3]: >>>> -t - n⋅t >>>> ──────── >>>> n >>>> >>>> This relatex to lines 153 and 154 of solvers/tests/test_solvers.py: >>>> assert solve_linear_system(M, x, y, z, t) == \ >>>> {y: 0, z: -(t+t*n)/n, x: -(t+t*n)/n} >>>> >>>> I don't know how to write the test so that sympy doesn't factor in the >>>> minus 1 in the numerator when it evalutes the assertion. >>>> >>>> A pretty version of my session can be found here: >>>> http://paste.blixt.org/115586 >>>> >>>> What is the right way to fix the test in test_solvers line 153? >>>> >>>> Thanks, >>>> >>>> Ryan >>>> >>>> In [1]: x, y, z, t, n = symbols('xyztn') >>>> >>>> In [2]: my_res = -(t+n*t)/n >>>> >>>> In [3]: my_res >>>> Out[3]: >>>> -t - n⋅t >>>> ──────── >>>> n >>>> >>>> In [4]: M = Matrix([[0,0,n*(n+1),(n+1)**2,0], >>>> ...: [n+1,n+1,-2*n-1,-(n+1),0], >>>> ...: [-1, 0, 1, 0, 0]]) >>>> >>>> In [5]: res = solve_linear_system(M, x, y, z, t) >>>> >>>> In [6]: res[z] >>>> Out[6]: >>>> -(t + n⋅t) >>>> ────────── >>>> n >>>> >>>> In [7]: res[z] == -(t+n*t)/n >>>> Out[7]: False >>>> >>>> In [8]: my_res = simplify(my_res) >>>> >>>> In [9]: my_res >>>> Out[9]: >>>> -(t + n⋅t) >>>> ────────── >>>> n >>>> >>>> In [10]: my_res = res[z] >>>> >>>> In [11]: my_res = -(t+n*t)/n >>>> >>>> In [12]: my_res == res[z] >>>> Out[12]: False >>>> >>>> In [13]: my_res = simplify(my_res) >>>> >>>> In [14]: my_res >>>> Out[14]: >>>> -(t + n⋅t) >>>> ────────── >>>> n >>>> >>>> In [15]: my_res == res[z] >>>> Out[15]: True >>>> >>>> >>> >>> >>> >>> >>> >>> >>> >> > > > > > > > > --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "sympy" group. To post to this group, send email to [email protected] To unsubscribe from this group, send email to [email protected] For more options, visit this group at http://groups.google.com/group/sympy?hl=en -~----------~----~----~----~------~----~------~--~---
