New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
2302 Canonical powers and fix to perfect_power #267
Conversation
TODO: wait for review |
Test results: http://pastebin.com/1tMs9kV3 Automatic review by sympy-bot. |
@asmeurer, can you give this a review? |
By the way, the commit from issue 415 was 7b87b62. |
num_rat.extend(grow) | ||
num_exp.append((bi, num_rat[i][1])) | ||
i += 1 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this code to be correct it is important that igcd always returns a positive number. (This seems to be the case but I don't see where that guarantee on igcd is documented).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this code to be correct it is important that igcd always returns
a positive number. (This seems to be the case but I don't see
where that guarantee on igcd is documented).
This is part of the igcd tests in test_numbers. But I added it to the docstring, too.
/c
This looks good to me. Perhaps some tests with negative bases might be a good idea. |
o perfect_power now returns False rather than None |
Test results html report: http://pastehtml.com/view/1ed2zqy.html Automatic review by sympy-bot. |
There seems to be a test failure in concrete/summations. |
What can be wrong? Nothing fails here.
Can anybody else confirm these failures? Is something strange going on? |
Well the failure in summation is actually a doctest, but I cannot reproduce it independently either. It only happens when I run setup.py test... I suppose it's some weird caching issue. Not that I know what to do about it ^^ |
Test results html report: http://pastehtml.com/view/1edeyx6.html Automatic review by sympy-bot. |
I get a few failures too in doctests. There seem to be some problem (e.g. wrong numbers returned), e.g. it's not just some hashing issue. |
A number like 2**5*3**3 was coming back as being a perfect power with exponent of 1, i.e. (2**5*3**3, 1). It should be False to indicate that it is not a perfect power.
When a power of a prime factor of a power's base shares a gcd with the denominator of the base's exponent, e.g. in (2**4*3)**(1/6) the 4 shares a gcd of 2 with 6, then that prime will be factored out of the base: e.g. to give 2**(2/3)*3**(1/6).
When a sequence of integer-based powers is multiplied, a canonical representation can be obtained by attempting to separate all relatively prime bases with different powers. So for 2**(1/3)*3**(1/4)*6**(1/5) the 2 (and 3) are extracted from the 6 and the powers combined to give 2**(8/15)*3**(9, 20)
Now the gcd work is done after doing all the base and exponent combining. This leads to fewer terms to process. In addition, -1 is pulled out of every Number-based power with a Rational exponent and either stands alone or is re-inserted into a power with a matching exponent.
Some Rationals are protected from making changes to p and q. Others are not. When you change the p and q values you are modifying that object in place and if that is a cached object then that object will no longer have the same value when it is recalled later.
Real(1.0) was being changed to S.One by flatten. This was removed since no other Real was being treated that way, e.g. -1.0 is retained. A couple of test were changed.
Bases having a common exponent should not combine unless the bases are positive or the exponents are integers. The exception being that a negative base or a base with an unknown sign can join non-negative bases.
Some condensation of code was done and the portion where c_exp was traversed while being deleted and reconstructed was re-written.
I think I found the problem and left the commit (don't modify .p and .q) next to the offending commit so as not to forget this nasty little error. Can you guys confirm that there are now no failures? |
Test results html report: http://pastehtml.com/view/1edn0kz.html Automatic review by sympy-bot. |
So the tests look good now [wester is not your fault]. |
OK, if there are no other objections this will be pushed in about 8 hours. |
it's in. |
2302: gcd of multiplied integer bases are extracted
See issue 2302 and issue 2321.