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 is_cannonial for polynomials #895

Merged
merged 2 commits into from Apr 2, 2016

Conversation

Projects
None yet
3 participants
@CodeMaxx
Copy link
Contributor

CodeMaxx commented Mar 30, 2016

Earlier UnivariateIntPolynomial allowed polynomials with symbol as ""(empty) and with powers greater than 0.

RCP<const UnivariateIntPolynomial> R = univariate_int_polynomial(none, {{5, 2_z}});

was treated as canonical and on printing as string gave 2*

Error inUnivariatePolynomial was that when symbol was ""(empty) and the dict was empty, they still checked for the first element of the dict(which did not exist).
So ```cpp
RCP S = univariate_int_polynomial(none, {});

was not being considered canonical due to which this code below gave an error
```cpp
RCP<const UnivariatePolynomial> c = univariate_polynomial(none, 0, {{0, 5}});
REQUIRE(c->diff(x)->__str__() == "0");

This was due to the following code executed in derivative.cpp when symbol was ""(empty)

map_int_Expr d;
return make_rcp<const UnivariatePolynomial>(self.get_var(), 0, std::move(d)); 
// empty symbol, empty dict

@isuruf @irislq @chenchfort @Sumith1896

@CodeMaxx CodeMaxx force-pushed the CodeMaxx:poly-canonical branch 2 times, most recently from 702e742 to c5587ff Mar 31, 2016

@@ -22,9 +22,8 @@ UnivariateIntPolynomial::UnivariateIntPolynomial(const RCP<const Symbol> &var, c

bool UnivariateIntPolynomial::is_canonical(const unsigned int &degree_, const map_uint_mpz& dict) const {
if (var_->get_name() == "")
if (!(dict.empty() or dict.size() == 1))
if (!((dict.empty() or dict.size() == 1) and (dict.empty() or dict.begin()->first == 0)))

This comment has been minimized.

@isuruf

isuruf Mar 31, 2016

Member

dict.empty() or (dict.size() == 1 and dict.begin()->first == 0) is simpler than above.

@CodeMaxx CodeMaxx force-pushed the CodeMaxx:poly-canonical branch from c5587ff to e9aa0a9 Mar 31, 2016

@CodeMaxx

This comment has been minimized.

Copy link
Contributor

CodeMaxx commented Mar 31, 2016

Ping @isuruf

@@ -347,8 +346,9 @@ UnivariatePolynomial::UnivariatePolynomial(const RCP<const Symbol> &var, const s

bool UnivariatePolynomial::is_canonical(const unsigned int &degree_, const map_int_Expr& dict) const {
if (var_->get_name() == "")
if (!((dict.empty() or dict.size() == 1) and dict.begin()->first == 0))
if (!((dict.empty() or dict.size() == 1) and (dict.empty() or dict.begin()->first == 0)))

This comment has been minimized.

@isuruf

isuruf Apr 1, 2016

Member

Same comment as above

@CodeMaxx CodeMaxx force-pushed the CodeMaxx:poly-canonical branch from e9aa0a9 to 83aef47 Apr 1, 2016

@irislq

This comment has been minimized.

Copy link
Contributor

irislq commented Apr 1, 2016

@isuruf @CodeMaxx CMake build issues

@CodeMaxx

This comment has been minimized.

Copy link
Contributor

CodeMaxx commented Apr 1, 2016

Travis is passing. See here https://travis-ci.org/CodeMaxx/symengine/builds/119992298 .

Some builds failed due to internal errors of Travis.

@irislq

This comment has been minimized.

Copy link
Contributor

irislq commented Apr 1, 2016

@CodeMaxx Ok, just wanted to make sure since it doesn't show up here

@CodeMaxx CodeMaxx force-pushed the CodeMaxx:poly-canonical branch from 99f8ae8 to a053727 Apr 1, 2016

@isuruf isuruf merged commit 6206f87 into symengine:master Apr 2, 2016

3 checks passed

codecov/project 74.81% remains the same compared to 419b812
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@isuruf

This comment has been minimized.

Copy link
Member

isuruf commented Apr 2, 2016

Thanks for the PR

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