Updates:
        Labels: -NeedsReview NeedsBetterPatch

Comment #20 on issue 1766 by Vinzent.Steinberg: expand(power_base=True) is too aggressive
http://code.google.com/p/sympy/issues/detail?id=1766

Wow, you accumulated a great amount of changes in your 1766 branch! I ignored the 2
'0000' commits. In general I'm +1 for the things implemented.

However I get several failures, see attachment. It's mainly about sets which are
tried to be subscripted, due to the change to iterables.py.

Here some comments about the commits (assuming the tests would pass):

1785 looks fine.

1789: Why did you leave all the commented code? Just remove it using another commit to your branch, so we can get it back using git if we ever want. There is also a very long line, please move the long list elsewhere (outside the function?) and break it. For performance, a set could be used instead, but I'm not sure whether it makes a difference. Why is the a print statement in the test file? Looks fine otherwise.

1778:I think we don't need decimal here and we could use mpmath, but that's not really important. And I'm not sure whether sympify() should use nsimplify(). But it
looks fine.

For the hint manager: I think the foo(anything=True) should set anything else to False(overwrite the defaults). foo(something=False) should only change this part,
although this is less likely to be needed. Same for foo(something=False,
anotherthing=True). But I think the first thing is much more intuitive.

I'm also not sure where to place it, but basic.py (manages calling etc.) seems more fitting to me than iterables.py. But it also fits to the utilities directory.

1766: Somewhat large, could be splitted. For example the changes to facts.py, extract_additively, separate, trigsimp, separatevars, powsimp and test_simplify. Given the changes were not necessary to make tests pass. EXP should be a dummy symbol.

Should we unify E**x and exp(x)?

The doctests for separate() could be preserved. There are some minor formatting issues like missing spaces around '=' in variable assignments (not kwargs of course).

The new functionality looks great!

If commit 1772 fixes something, it lacks a test.

1794 looks fine.

1795 too, except the missing spaces around '==' and '+'. And there are lines much too
long.


Attachments:
        1766failures.txt  43.1 KB

--
You received this message because you are listed in the owner
or CC fields of this issue, or because you starred this issue.
You may adjust your issue notification preferences at:
http://code.google.com/hosting/settings

--
You received this message because you are subscribed to the Google Groups 
"sympy-issues" group.
To post to this group, send email to sympy-iss...@googlegroups.com.
To unsubscribe from this group, send email to 
sympy-issues+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/sympy-issues?hl=en.

Reply via email to