1858b (1811) separatevars modifications and noncommutative factoring #422
Conversation
|
I would have expected that I can just use factor() and it would call factor_nc() when necessary. |
|
Yes...but first I need to get factor_nc in place. One change at a time, |
| expr = expr.subs(rep) | ||
| return expr, dict([(v, k) for k, v in rep]) or None, nc_syms | ||
|
|
||
| def factor_nc(expr, **kwargs): |
There was a problem hiding this comment.
Shouldn't this go to sympy/core/expr.py ? That's where factor() is defined.
There was a problem hiding this comment.
Shouldn't this go to sympy/core/expr.py ? That's where factor() is defined.
I can move it there.
|
Perhaps http://arxiv.org/abs/1002.3180 could be useful here. I didn't read it too deeply, but it doesn't seem complicated. Feel free to open an issue for it if you don't want to work on it now. |
|
That paper also shows that non-commutative factorization is not unique in the multivariate case. They give the example: In [23]: x, y = symbols('x y', commutative=False)
In [24]: expand((y*x + 1)*(y*x*y - y))
Out[24]: -y + y⋅x⋅y⋅x⋅y
In [25]: expand((y*x - 1)*(y*x*y + y))
Out[25]: -y + y⋅x⋅y⋅x⋅y |
|
I'll open an issue. Thanks for the ref, it does look useful. |
|
OK, this is ready to go from my end. |
|
All 2.7 tests have passed. |
|
The factorial simplification test was moved to the factorial test suite. |
|
@jrioux do you plan to finish reviewing this? |
|
Sure -- The work looks good to me, and last time I checked the tests pass on my machine (python 2.7.0). Not sure what "finish reviewing this" involves though. Are there contributor guidelines posted somewhere? I wouldn't mind helping to get this pushed. |
|
Nothing particularly surprising. Just go through the code and make sure it looks OK. Make sure that everything is well tested (especially any new features or bug fixes). New public functions should have docstrings with doctests (I don't think that's the case here). Finally, make sure the tests pass. SymPy-Bot is a good way to do that. The point to reviewing is to get a second set of eyes on the code. There's no checklist of things to look for for each one (if the process could be completely automated, it would be). What you've done so far is basically all there is to it. If you're worried about missing something, just check the added tests, because that's our #1 force against regression. |
|
tests fail, NameError: |
|
XFAIL is imported; this was rebased on master. All 32-bit, py 2.7 tests pass. |
|
The two "fix tests" commits should probably be squashed with other commits in order not to gratuitously break git bisect, but otherwise this looks good to me. |
no tests fail without this; the full expansion is only
done with the factor step
factoring of noncommutatives fails so this should be avoided
If the expression is univariate, there is nothing to separate
(though factoring might change things, this is not the purpose
of this routine).
Just as the routine need not start when the whole expression is
univariate, it also does not need to end with factoring if the
nonseparated terms are univariate.
These functions can be used when looking for common
noncommutative sequences.
factor_nc was modified to handle more nc expressions; tests
were added for this.
|
I did squash some together. An option to know about when bisecting is Thanks for finishing up this review. This is now in. |
1858b (1811) separatevars modifications and noncommutative factoring
Changes basically following those described at http://code.google.com/p/sympy/issues/detail?id=1811 are contained in these commits. In addition, iterable functions common_prefix and common_suffix are added to handle identification of extractable sequences in noncommutative terms and are used in a noncommutative factoring routine.
I've left these as a series of individual commits hoping that this might help in the review process (to keep the individual changes per commit small).
After committing, search issues for this pull request number (e.g. put "422" in the search box to see which might have been related to this issue.)