I wrote up a function that does this. It is separate from expand (I called it distribute).  It works slower than 
expand does, but it only distributes multiplication over addition and only does so on the top level.  I am not sure 
how easy it would be to have expand do this and only do it on the top level, as expand works through class 
functions.  

Attached is a patch with the function I wrote up.  

0001-Added-function-distribute-which-distributes-multipl.patch 4.7 KB   Download

>>> e = log(x**2)*(a+b)
>>> e._eval_expand_<TAB>
e._eval_expand_basic(    e._eval_expand_complex(  e._eval_expand_func(     
e._eval_expand_power(    e._eval_expand_trig(
>>> e._eval_expand_basic()
2*a*log(x) + 2*b*log(x)
>>> e._eval_expand_power()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "sympy/core/basic.py", line 1398, in _eval_expand_power
    if not isinstance(self, C.Apply):   # FIXME Apply -> Function
  File "sympy/core/basic.py", line 2354, in __getattr__
    raise AttributeError("No SymPy class '%s'" % name)
AttributeError: No SymPy class 'Apply'

Maybe there should be _eval_expand_log?

+1 for a more subtle expand()

I am -1. The patch itself is good, but looking at the tests, the only difference
between distribute() and expand() is this:

In [9]: expand((log(x**2)+log(y))*z)
Out[9]: z⋅log(y) + 2⋅z⋅log(x)

distribute() returns log(x**2) here instead of 2*log(x). I think expand() should have
an option what things to expand and once we fix:

In [11]: log(x**2).expand(log=True)
Out[11]: 2⋅log(x)

In [12]: log(x**2).expand()
Out[12]: 
   ⎛ 2⎞
log⎝x ⎠

Then expand() will do what you want. No need to maintain yet another expansion routine.

What do you think?

I agree with Ondrej, this is what I meant in my comment. expand() should not expand
things the user does not want to be expanded, and there is no need for another method.

I agree that it would be nicer to have this all in expand, especially since this is slower than expand (see the long test), but 
my question is, would it be possible for expand to only distribute on the top level (as my function does), since expand just 
recursively calls class methods?  How would that be implemented?  I need the ability not only to just distribute 
multiplication, but to also to only do it on the top level.  

Here is another test that demonstrates what I mean.  Also, once I am finished with  issue 252 , expand also seems to expand 
exp(x+y) into exp(x)*exp(y), so that is another thing it would be nice if it did separately (probably should make the defunct 
expand_power do this).  

Also, are there any other things that expand_basic does that has not been mentioned here?  So far we have:
- Distribute multiplication over addition
- Pull exponents out of logs
- Expand added exponents if it is given the change (i.e., automatic combining of exponents is not enabled)
- Do all of these on all levels.

0001-Added-one-more-test-to-distribute-that-demonstrates.patch 1.1 KB   Download

I think it should be fairly simple to implement expand(recursive=False). Just cancel
before calling anything recursively.

There is also trigonometric expansion:

>>> (sin(2*x)).expand(trig=True)
2*cos(x)*sin(x)

But this is not done by default.

So here is the way that I plan on doing this.  Let me know if this is the best.  

I checked, and expand_basic only does the three things that I mentioned above (and power expansion currently only works for exponentials.  I 
will need to write that into pow).  expand_basic in Pow also expands things like (x+y)**3 and (x*y)**n to x**n*y**n.  Do we want these further 
separated, or would it be reasonable to put those, along with x**(a+b) to x**a*x**b all in expand_power?  

I want to retain backwards compatibility with expand_basic, so I plan on writing expand_power, expand_log, and expand_distribute (is this the 
best name for this?) in their respective functions, and then making the expand_basic function just a wrapper that calls them.  The only 
disadvantage of doing this is that you would have to either do expand(expr, basic=False, power=True), or hard code the three methods 
expand_basic calls into expand() so that it know that power=True also means basic=False, which doesn't seem to be the way that it is written.  
Currently, expand() is modular in that it calls hint functions without any prior knowledge of which functions should or should not exist.

The advantage of doing this is that basic=True, the default, would work in the identical way that it does now, except for the power expansion 
that I plan on adding.  

So is wrapping the functions around expand_basic the best way of doing this, or is there a cleverer way that I haven't though of?  Namely, is 
there any way to make expand know that things like power=True imply basic=False without hardcoding them into expand()?  There is probably 
some python feature that does this that I am not familiar with.  

What about getting rid of expand_basic and hardcoding reasonable defaults if no
keyword is specified? Or just call _expand_basic() if there's no keyword (more
general, the object has to decide what "basic" means).

I think Ondrej and I decided an a suitable course of action.  See this thread: http://groups.google.com/group/sympy/browse_thread/thread/3fb7a3036c0b9d87

I don't like this _expand_basic at all, I think we all seem to agree on that. 

_expand_basic has advantages:

If expand() with no keyword arguments just calls _expand_basic, the user can define
his own expansion rules for his objects and it will just work with expand().
Hardcoding some defaults does not allow this.

I could just leave in basic as a default argument but not have any SymPy object implement it.  Then if someone 
wants to have their custom expand work by default but it doesn't fit one of the default expand names, then they 
could call it expand_basic or implement an expand_basic wrapper around whatever expands they want to run by 
default.  

Exactly.

basic is also useful for things like  issue 1445  where you want to implement expand on a class like Integral.  I did 
it so Integral just takes in the hints through expand_basic and expands the integrand.  

Nice! Integral.expand() could also expand constant factors (Sum.expand() too), but
this probably has to wait for the new assumptions system.

Here is what I have.  See the commit message for more info.  

You can also pull from http://github.com/asmeurer/sympy/tree/expand

This also fixes  issue 1453  and  issue 1445 .  

0001-Refactored-expand-fixing-issues-1455-1453-and-1445.patch 64.8 KB   Download

I'm very positive about the functionality and the tests.

There is a massive amount of code duplication, the following snippet occurs many times:

+        sargs, terms = self.args[:], []
+        for term in sargs:
+            try:
+                newterm = term._eval_expand_complex(recursive=recursive, **hints)
                                             ^^^^^^^ only thing that changes
+            except AttributeError:
+                newterm = term
+            terms.append(newterm)
+        return self.new(*terms)

If something does not implement _eval_expand..., it should just inherit it from it's
parent class (Maybe this is already done this way, I'm not sure. The code should be
less redundant though).

I'm not sure about all these expand_...() functions polluting the namespace. I'd
prefer a single expand() with keyword arguments. Maybe this should be even used for
_eval_expand_... Being private this is not as important, but this would help reducing
code duplication. This would be imho an improvement over the current way to implement
expansion.

I'm getting these failures with you branch:

_________________ sympy/series/tests/test_nseries.py:test_bug2 _________________
  File "sympy/series/tests/test_nseries.py", line 126, in test_bug2
    assert e.nseries(w,0,4).subs(w,0)==3
  File "sympy/core/basic.py", line 2088, in nseries
    return self._eval_nseries(x, x0, n)
  File "sympy/core/add.py", line 184, in _eval_nseries
    terms = [t.nseries(x, x0, n) for t in self.args]
  File "sympy/core/basic.py", line 2088, in nseries
    return self._eval_nseries(x, x0, n)
  File "sympy/core/mul.py", line 760, in _eval_nseries
    terms = [t.nseries(x, x0, n) for t in self.args]
  File "sympy/core/basic.py", line 2088, in nseries
    return self._eval_nseries(x, x0, n)
  File "sympy/core/power.py", line 650, in _eval_nseries
    raise NotImplementedError()
NotImplementedError

________________________________________________________________________________
_______________ sympy/series/tests/test_nseries.py:test_issue364 _______________
  File "sympy/series/tests/test_nseries.py", line 237, in test_issue364
    assert e.nseries(w, 0, 1) == e_ser
AssertionError
________________________________________________________________________________
_______ sympy/utilities/tests/test_code_quality.py:test_implicit_imports _______
  File "sympy/utilities/tests/test_code_quality.py", line 108, in test_implicit_imports
    check_directory_tree_imports(sympy_path, exclude)
  File "sympy/utilities/tests/test_code_quality.py", line 88, in
check_directory_tree_imports
    assert False, message_implicit % (fname, idx+1)
AssertionError: File contains an implicit import:
sympy/sympy/core/tests/test_basic.py, line 633.

Those are related to the exponential combining patches from  issue 1445 .  Hopefully, I will be able to get those 
fixed soon.  If you just apply my patch, you should not see those failures (though you may see others, because 
some of the code is based on exponentials not automatically combining.  

IIRC, I can't apply your patch without those from  issue 1445 , so I just tried your
branch.

I have finally finished this!  Thanks to Ondrej for helping me fix those nseries tests.  Here is a final patch ready 
for review.  This also fixes the issues mentioned above and in the commit logs, in particular  issue 252 .  I 
rebased with master, so there shouldn't be any problems pulling.  

Please review:
http://github.com/asmeurer/sympy/commits/expand-exp-review

I commented some commits of your branch on github (only minor things). (I also
verified that all test work.)

More important to me is what I wrote in comment 18 of this issue.

Indeed, I also don't like this commit:

http://github.com/asmeurer/sympy/commit/1e6d88cba99eb9983d3c2650262d01b998ac868a

also I don't like the try/except AttributeError, clause. It catches *any*
AttributeError, and it will just hide bugs. Let's write this explicitely.

Aaron, is there a way to address the comment 18? I agree with Vinzent, so if you
cannot figure something out, ping me and I'll try to.

Otherwise the patches are +1.

Removing the AttributeError will be easy.  I just need to use hasattr instead.  I will push a fix for that to the branch.

As for comment 18, those functions are there in mul, add, pow, and function to inherit from.  These need to be different from the inheritance from Basic.  
The Basic._eval_expand... functions return self, whereas the mul, add, power, and function ones either expand if there is an expansion for it (such as 
power_exp for power), or they recurse through the arguments and expand each of them (if deep is True).  I was originally going to keep it like it currently is 
in Basic._eval_expand_basic(), but that takes self.args and then calls self.new(self.args.expand()), which fails for many classes like Poly() which do not take 
the same arguments as their .args attribute gives.  

This is really the same as it was, only now there are nine _eval_expand functions instead of four, and they now return self instead of None when there is no 
expansion (actually, only expand_basic returned None.  Try expand(Poly(x**2 + 1, x) and expand(Poly(x**2 + 1, x, func=True) in the trunk).  I tried working 
around this several ways, but found that just making Basic expand functions return self and have Add, Mul, Power, and Function recurse through the 
arguments was the cleanest, least hacky, and the only one that really worked.  

See also the thread linked to in comment 9.  Fabian suggested using an Expand() class similar to the printer, and Ondrej said that we should do after my 
patches are in.  

I suggest to get this in and improve later. I think it's good enough and maintainable.

Unless of course anyone (Vinzent? :) can find a better solution now.  Fix the
try/except thing and then let's get this in.

I propose to use a single expand method.

    def _eval_expand(self, what='basic', deep=True, **hints):
        sargs, terms = self.args[:], []
        for term in sargs:
            newterm = term._eval_expand(deep=deep, **hints)
            terms.append(newterm)
        return self.new(*terms)

The type of expansion is another argument (or just a hint if you want). I'm sure
there is a better name than 'what'. :)

When you reimplement _eval_expand, you just check for "what" (instead of strings we
could also use constants):

    if what == 'basic':
        ...
    elif what == 'mul':
        ....
    else:
        return self # or raise an exception

I think this would reduce a lot of code duplication and subclassing should be easier,
because there is less overhead when implementing the necessary expand methods.
(BTW: The same approach is used by Qt's model and view concept)

Same thing for expand() of course.


If you think this is too much effort we can get it in and improve the code later (I'm
volunteering for this :), because Aaron needs the functionality for his GSoC project.

It would be more flexible if "what" can be a list, so maybe rather

    what = set(what)
    if 'mul' in what:
        ...

How does this work if someone wants to implement their own expand method for a class that they wrote?  With 
my patch, they just have to write an _eval_expand_whatever function and expand will call it if whatever=True is 
given as a hint.  Or they could write it in _eval_expand_basic or _eval_expand_(mul, log, ...) if it fits and expand 
will call it by default.  It seems like with your idea, either Basic.expand has to know about all possible expand hints 
or any subclass expand function has to properly write in all of the default hints (mul, log, etc.).  If that is true than 
I don't like it.

We need to have Basic._eval_expand() return self and override it in Mul, Add, Pow, and Function.  This is because 
self.new(*terms) as in your code above does not work always (see comment 25 above).  Alternatively, we can check 
if it is a Mul, Add, Pow, or Function and expand the args, otherwise, return self.  The former is purer with regards 
to object oriented design, but the later would mean less duplicated code.  

expand() is just a wrapper that sets up default hints and then calls expr.expand().  

Why do we need "what" to be a set?  If I understand correctly, what is just a key of the current expand method.  
expand() just goes through the keys in hints and calls _eval_expand_hint in my patch.  It looks like with your way 
it would just cal _eval_expand(what='hint').  I think "current" is a better name than "what".

If we end up waiting, we should also consider Fabian's Expand class idea. 

Let's commit your code, I'll give it a try later. Showing code is better than talking
about it. :)

Does basic have default hints a subclass can inherit currently? Looking at your code,
you seem to reimplement the default hints for any subclass. I don't see the use of
the advantage of inheritance. What's the problem with implementing the default hints
in Basic (or its expand method)?

A subclass has only to catch the hints it wants to reimplement, the rest should be
handled by the parent, leading to "default" behavior.

expand() could call _eval_expand() of the subclass, and if it returns None, it could
call the parent's _eval_expand() and so on. (Speaking of expand() I also mean
expr.expand(), they are equivalent.)

> Why do we need "what" to be a set?

If it should be an own argument. If we just use hints, there is no need for this.

Ok, I suggest to get aaron's patches in --- aaron, where is your latest branch? I'll
do final review and push it in, if all is ok.

(No comment was entered for this change.)

(No comment was entered for this change.)