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

Fixed bug in Polynomial derivative #894

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@CodeMaxx
Copy link
Contributor

CodeMaxx commented Mar 30, 2016

Earlier diff() gave derivative even w.r.t. ""(empty symbol)
So

Symbol none = symbol("");
RCP<const UnivariateIntPolynomial> b = univariate_int_polynomial(none, {{0, 1_z}});
REQUIRE(b->diff(none)->__str__() == "0"); // Returned false

gave error.

Also for

b = univariate_int_polynomial(x, {{0, 1_z}, {1, 2_z}});

b->diff(none) gave 0

@irislq @isuruf @chenchfort @Sumith1896

@CodeMaxx CodeMaxx force-pushed the CodeMaxx:derivative branch from 0e06364 to e7fd9bf Mar 30, 2016

@irislq

This comment has been minimized.

Copy link
Contributor

irislq commented Mar 30, 2016

Nice, didn't think about this situation before. +1 from me.

@@ -437,6 +437,9 @@ static RCP<const Basic> diff(const CLASS &self, \

static RCP<const Basic> diff(const UnivariateIntPolynomial &self,
const RCP<const Symbol> &x) {
if(x->__eq__(*symbol("")))
throw:: std::runtime_error("Derivative can't be with respect to nothing");

This comment has been minimized.

@isuruf

isuruf Mar 31, 2016

Member

throw:: -> throw

This comment has been minimized.

@CodeMaxx

CodeMaxx Mar 31, 2016

Contributor

I wonder why it didn't give a compiler error

@CodeMaxx CodeMaxx force-pushed the CodeMaxx:derivative branch from e7fd9bf to 3537d26 Mar 31, 2016

@irislq

This comment has been minimized.

Copy link
Contributor

irislq commented Mar 31, 2016

@isuruf Looks like one of the Travis builds terminated prematurely. Can someone restart Travis?

@Sumith1896

This comment has been minimized.

Copy link
Member

Sumith1896 commented Mar 31, 2016

@irislq Done

@CodeMaxx

This comment has been minimized.

Copy link
Contributor

CodeMaxx commented Mar 31, 2016

@isuruf Ping

@CodeMaxx CodeMaxx force-pushed the CodeMaxx:derivative branch from 185a3ae to 715d3e6 Apr 1, 2016

@irislq

This comment has been minimized.

Copy link
Contributor

irislq commented Apr 3, 2016

@CodeMaxx Can you solve the merge conflicts?

@isuruf

This comment has been minimized.

Copy link
Member

isuruf commented Apr 3, 2016

This issue is not unique to polynomial derivative. Checking that the symbol is not equal to "" is an extra overhead in each call to diff which is not really needed.

@isuruf

This comment has been minimized.

Copy link
Member

isuruf commented Apr 3, 2016

What was the result of,

Symbol none = symbol("");
RCP<const UnivariateIntPolynomial> b = univariate_int_polynomial(none, {{0, 1_z}});
b->diff(none)->__str__()

before?

@CodeMaxx

This comment has been minimized.

Copy link
Contributor

CodeMaxx commented Apr 11, 2016

@isuruf Earlier there was an error since there was an error in is_canonical() which was fixed by the other PR.

@CodeMaxx CodeMaxx closed this Apr 11, 2016

@CodeMaxx CodeMaxx reopened this Apr 11, 2016

@isuruf

This comment has been minimized.

Copy link
Member

isuruf commented Apr 11, 2016

Ah okay. See my comment here, #894 (comment)

@CodeMaxx

This comment has been minimized.

Copy link
Contributor

CodeMaxx commented Apr 11, 2016

@isuruf Are you sure its not required? I mean the user is not likely to do it directly but in complex computations where variables are passed around(Polynomials are used in all kinds of computations), this situation may arise due to some error by the user... The user will be able to detect this more easily if we raise an exception.

@CodeMaxx CodeMaxx closed this Apr 23, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment