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

Use from_dict() in UnivariatePolynomial. #896

Merged
merged 5 commits into from Apr 5, 2016

Conversation

Projects
None yet
3 participants
@CodeMaxx
Copy link
Contributor

CodeMaxx commented Mar 30, 2016

Earlier the constructor was called directly.

This prevents terms with zero coefficient to go into the polynomial.
Also prevents redundant pass of degree.


Also changed the from_dict() function to allow for polynomial with value zero.

@isuruf @irislq @Sumith1896

{
auto iter = d.begin();
while (iter != d.end()) {
if (Expression(0) == iter->second) {
if (Expression(0) == iter->second and Expression(0) != iter->first) {

This comment has been minimized.

@CodeMaxx

CodeMaxx Mar 30, 2016

Contributor

@isuruf I have a doubt. This change is required for this test case to work. Without this z->is_zero() and z->is_integer() return false. But this change is not required for a similar test case in UnivariateIntPolynomial. Why so?

This comment has been minimized.

@irislq

irislq Mar 31, 2016

Contributor

@CodeMaxx Can you do one thing and change UnivariatePolynomial::is_zero() to return just dict_.empty()? Maybe that'll help.

@irislq

This comment has been minimized.

Copy link
Contributor

irislq commented Mar 31, 2016

@CodeMaxx Another thing is that in derivative.cpp, make_rcp<const UnivariatePolynomial> is used. I think this needs to be changed to from_dict() (like in UnivariateIntPolynomial) or this univariate_polynomial()

@CodeMaxx CodeMaxx force-pushed the CodeMaxx:poly-use-from_dict branch from c9b048f to 6c75b1d Mar 31, 2016

@CodeMaxx

This comment has been minimized.

Copy link
Contributor

CodeMaxx commented Mar 31, 2016

I rather think, use of from_dict() should be removed from UnivariateIntPolynomial also since the way diff() works , the checks done in from_dict() are not required. So call to from_dict() will be redundant.

@CodeMaxx CodeMaxx force-pushed the CodeMaxx:poly-use-from_dict branch from 6c75b1d to fc292ab Mar 31, 2016

@CodeMaxx

This comment has been minimized.

Copy link
Contributor

CodeMaxx commented Mar 31, 2016

@isuruf Ping

@irislq

This comment has been minimized.

Copy link
Contributor

irislq commented Apr 1, 2016

@isuruf @Sumith1896 Cmake issues in AppVeyor

@CodeMaxx CodeMaxx force-pushed the CodeMaxx:poly-use-from_dict branch from 17b9465 to e7e09c0 Apr 1, 2016

@@ -204,10 +204,9 @@ RCP<const UnivariatePolynomial> mul_uni_poly(RCP<const UnivariatePolynomial> a,
RCP<const UnivariatePolynomial> b);

inline RCP<const UnivariatePolynomial>
univariate_polynomial(RCP<const Symbol> i, unsigned int deg,
const map_int_Expr &&dict)
univariate_polynomial(RCP<const Symbol> i, map_int_Expr &&dict)

This comment has been minimized.

@irislq

irislq Apr 3, 2016

Contributor

Why did you remove the const for dict?

This comment has been minimized.

@CodeMaxx

CodeMaxx Apr 3, 2016

Contributor

from_dict() uses erase to remove member of dictionary with coefficients 0 . This will not be possible for a const dictionary

@irislq

This comment has been minimized.

Copy link
Contributor

irislq commented Apr 3, 2016

@CodeMaxx Could you fix the merge conflicts?

@CodeMaxx

This comment has been minimized.

Copy link
Contributor

CodeMaxx commented Apr 3, 2016

Fixed

@isuruf

This comment has been minimized.

Copy link
Member

isuruf commented Apr 3, 2016

Binary files have been committed. You need to remove them from history

@CodeMaxx CodeMaxx force-pushed the CodeMaxx:poly-use-from_dict branch from 2aad284 to 7b02e5f Apr 3, 2016

@CodeMaxx

This comment has been minimized.

Copy link
Contributor

CodeMaxx commented Apr 3, 2016

Done

@irislq

This comment has been minimized.

Copy link
Contributor

irislq commented Apr 4, 2016

@isuruf Is this ready to be merged yet?

@isuruf

This comment has been minimized.

Copy link
Member

isuruf commented Apr 5, 2016

@CodeMaxx, you need to remove the binary files from history since they are very large files

@CodeMaxx

This comment has been minimized.

Copy link
Contributor

CodeMaxx commented Apr 5, 2016

@isuruf I already removed them 2 days ago

@isuruf

This comment has been minimized.

Copy link
Member

isuruf commented Apr 5, 2016

Yes, but they are in the git commit history. If we merge this PR, then the size of the repo will be increased by around 5 times

@CodeMaxx CodeMaxx force-pushed the CodeMaxx:poly-use-from_dict branch from 7b02e5f to d18c051 Apr 5, 2016

@CodeMaxx

This comment has been minimized.

Copy link
Contributor

CodeMaxx commented Apr 5, 2016

@isuruf removed them from history

@isuruf

This comment has been minimized.

Copy link
Member

isuruf commented Apr 5, 2016

Looks good to me. Let's wait until tests pass

@isuruf isuruf closed this Apr 5, 2016

@isuruf isuruf reopened this Apr 5, 2016

@isuruf isuruf merged commit 70e0188 into symengine:master Apr 5, 2016

2 checks passed

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 5, 2016

@CodeMaxx, thanks for the PR

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