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

Added support for polynomials with negative degree #897

Merged
merged 6 commits into from Apr 6, 2016

Conversation

Projects
None yet
3 participants
@CodeMaxx
Copy link
Contributor

CodeMaxx commented Mar 31, 2016

  • Makes polynomials like x**(-1) + 2*x**(-2) valid
  • Changed printing for negative exponent terms from x**-1 to x**(-1)
  • Add more tests for derivative of negative exponent polynomials etc.

@isuruf @Sumith1896

@CodeMaxx CodeMaxx force-pushed the CodeMaxx:neg-degree branch from 167da1d to f54007d Mar 31, 2016

@CodeMaxx

This comment has been minimized.

Copy link
Contributor

CodeMaxx commented Apr 1, 2016

@isuruf Any specific tests you think should be added?

@irislq

This comment has been minimized.

Copy link
Contributor

irislq commented Apr 1, 2016

@isuruf @Sumith1896 Looks like some builds failed because of cmake install issues. Will restarting them solve this?

@CodeMaxx

This comment has been minimized.

Copy link
Contributor

CodeMaxx commented Apr 1, 2016

@irislq

This comment has been minimized.

Copy link
Contributor

irislq commented Apr 1, 2016

@isuruf If we can get this merged ASAP, it'll help the UnivariateSeries PR greatly, along with the from_dict(), diff() and is_canonical() fixes.

@CodeMaxx CodeMaxx force-pushed the CodeMaxx:neg-degree branch 2 times, most recently from 9b37bed to f56cff3 Apr 1, 2016

@irislq

This comment has been minimized.

Copy link
Contributor

irislq commented Apr 3, 2016

@CodeMaxx Merge conflicts, can we get this fixed? Thanks!

@CodeMaxx

This comment has been minimized.

Copy link
Contributor

CodeMaxx commented Apr 3, 2016

Fixed

x_pow = pow_ex(x, last_deg);
result = expand(result * x_pow);

return result;

This comment has been minimized.

@isuruf

isuruf Apr 3, 2016

Member

I;m not really sure this would be faster for symbolic polynomials. Also, expand calls can be expensive

This comment has been minimized.

@CodeMaxx

CodeMaxx Apr 3, 2016

Contributor

expand is absolutely necessary if we want to use Horner's scheme. What do you suggest for this then?

This comment has been minimized.

@isuruf

isuruf Apr 3, 2016

Member

Keep it as it was?

This comment has been minimized.

@CodeMaxx

CodeMaxx Apr 3, 2016

Contributor

Thats one options. Or we can benchmark it first and then decide?

This comment has been minimized.

@CodeMaxx

CodeMaxx Apr 3, 2016

Contributor

btw I commited this change by mistake. This is part of the Horner's Scheme PR

This comment has been minimized.

@isuruf

isuruf Apr 3, 2016

Member

Let me give you an example where it will surely be slower.
Consider the below example of a polynomial in x
((1+y)**1000)*x + 1
Evaluating the polynomial at x=1 will take more time in your method.

This comment has been minimized.

@CodeMaxx

CodeMaxx Apr 3, 2016

Contributor

Right.... so we don't want the coefficients of x to expand. I don't think this can be selectively done with expand? Otherwise we can keep the previous implementation.

@CodeMaxx CodeMaxx force-pushed the CodeMaxx:neg-degree branch from c28deab to 63efc7c Apr 3, 2016

@irislq

This comment has been minimized.

Copy link
Contributor

irislq commented Apr 5, 2016

@CodeMaxx Merge conflicts again.

@CodeMaxx

This comment has been minimized.

Copy link
Contributor

CodeMaxx commented Apr 6, 2016

Fixed

@isuruf

This comment has been minimized.

Copy link
Member

isuruf commented Apr 6, 2016

+1 to merge after tests pass.
Sorry about the merge conflicts.

@isuruf isuruf merged commit 6b5aeb1 into symengine:master Apr 6, 2016

3 checks passed

codecov/project 74.94% (+0.01%) compared to 70e0188 at 74.93%
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment