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

Improved abs #835

Merged
merged 8 commits into from Mar 15, 2016

Conversation

Projects
None yet
3 participants
@CodeMaxx
Copy link
Contributor

CodeMaxx commented Feb 28, 2016

Fix for #834
@Sumith1896 @isuruf

  • abs(y-x) is changed to abs(x-y)
  • make abs return evaluated values for all numerical types

CodeMaxx added some commits Feb 28, 2016

@isuruf

This comment has been minimized.

Copy link
Member

isuruf commented Feb 28, 2016

Thanks for working on this.
Does this work like SymPy?

In [1]: from sympy import *

In [2]: var("x y z")
Out[2]: (x, y, z)

In [3]: abs(x-y+1)
Out[3]: Abs(x - y + 1)

In [4]: abs(y-x-1)
Out[4]: Abs(x - y + 1)
@CodeMaxx

This comment has been minimized.

Copy link
Contributor

CodeMaxx commented Feb 28, 2016

No not like sympy
For e.g.
eq(*abs(sub(x,y)),*abs(neg(sub(x,y)))); returns true
while
eq(*abs(sub(x,y)),*abs(sub(y,x))); returns false (How do you suggest we handle this?)

I'm not exactly sure how could_extract_minus works but its returning false for y-x and true for -(x-y).

@isuruf

This comment has been minimized.

Copy link
Member

isuruf commented Feb 28, 2016

You can look at the code of could_extract_minus to see what happens. It returns true for Muls with coefficient negative or negative Numbers.

Have a look at how SymPy selects x - y + 1 over -x + y -1 and implement here.

@CodeMaxx

This comment has been minimized.

Copy link
Contributor

CodeMaxx commented Feb 28, 2016

@isuruf Added the code for Complex. Can review.

} else if (is_a<Complex>(*arg)) {
RCP<const Complex> arg_ = rcp_static_cast<const Complex>(arg);
RCP<const Complex> conjugate = rcp_static_cast<const Complex>(sub((*arg_).real_part(),mul(I,(*arg_).imaginary_part())));
return sqrt(abs(((*arg_).mulcomp(*conjugate))));

This comment has been minimized.

@isuruf

isuruf Feb 28, 2016

Member

abs call here is not needed.
Btw, sqrt(real*real+imaginary*imaginary) seems simpler than this.

This comment has been minimized.

@CodeMaxx

CodeMaxx Feb 28, 2016

Contributor

Writing sqrt(real*real+imaginary*imaginary) in terms of mul(), real_part() etc. also led to a long statement. Also I felt that the return statement looked clearer in the present case.

abs call removed. 👍

This comment has been minimized.

@isuruf

isuruf Feb 28, 2016

Member

I think sqrt(real*real+imaginary*imaginary) is faster because you are dealing with mpq_classs

@CodeMaxx

This comment has been minimized.

Copy link
Contributor

CodeMaxx commented Feb 28, 2016

@isuruf Sympy is using signsimp function being called from here ,for y-x+1 thing.
Its a general function and is being used in lots of places apart from abs. Do we have such a function in Symengine? If not how do I implement it?... I tried to understand sympy's code but couldn't understand it fully...will require some help on this.This is the first time I'm trying to port something from sympy.

@isuruf

This comment has been minimized.

Copy link
Member

isuruf commented Feb 28, 2016

It's not there in SymEngine. I don't know the SymPy code as well, but you can look at code and come to the place where the actual logic is. For example signsimp calls, sub_pre, which in turn calls could_extract_minus_sign which has a handling for Add which is not there in SymEngine.

@CodeMaxx

This comment has been minimized.

Copy link
Contributor

CodeMaxx commented Feb 28, 2016

@isuruf This thing has lots of layers. could_extract_minus further calls sort_key which further calls default_sort_key . I think its not a good idea to add all this to Abs class. We should think of a proper structure for this in symengine and develop similar layered code .
We should also take @certik 's opinion on this.

@isuruf

This comment has been minimized.

Copy link
Member

isuruf commented Feb 29, 2016

Default sort key in SymEngine is RCPBasicKeyLess. You don't have to change abs, just could_extract_minus.

Let me know if you need to know anything more

@@ -2346,6 +2347,9 @@ TEST_CASE("Abs: functions", "[functions]")
RCP<const Symbol> x = symbol("x");
RCP<const Symbol> y = symbol("y");

REQUIRE(eq(*abs(add(i2,mul(I,im1))),*sqrt(integer(5))));

This comment has been minimized.

@isuruf

isuruf Mar 10, 2016

Member

Can you fix the formatting here? Space after ,

@CodeMaxx CodeMaxx force-pushed the CodeMaxx:abs branch from 5560078 to fd4dfe2 Mar 10, 2016

@isuruf

This comment has been minimized.

Copy link

isuruf commented on symengine/functions.cpp in fd4dfe2 Mar 13, 2016

You should use arg_.real_ and arg.imaginary_ here

This comment has been minimized.

Copy link
Owner

CodeMaxx replied Mar 14, 2016

Why is such a change necessary?

I think you mean arg_->real_?

This comment has been minimized.

Copy link

isuruf replied Mar 14, 2016

You should use the rational_class types whenever possible inside algorithms, because it is faster

This comment has been minimized.

Copy link
Owner

CodeMaxx replied Mar 14, 2016

Cool. Updated.

@isuruf

This comment has been minimized.

Space after ,

@isuruf

This comment has been minimized.

Space after ,

CodeMaxx added some commits Mar 14, 2016

@isuruf

This comment has been minimized.

Copy link
Member

isuruf commented Mar 15, 2016

Looks good. Can you clean up the history a bit?

@CodeMaxx

This comment has been minimized.

Copy link
Contributor

CodeMaxx commented Mar 15, 2016

@isuruf I've not yet implemented abs(y-x) to abs(x-y) . Also I didn't exactly understand what you mean by cleaning up the history.

@Sumith1896

This comment has been minimized.

Copy link
Member

Sumith1896 commented Mar 15, 2016

@CodeMaxx Reduce the number of commits and improve messages

@CodeMaxx

This comment has been minimized.

Copy link
Contributor

CodeMaxx commented Mar 15, 2016

@Sumith1896 Squashing commits is creating problem due to the merges with upstream. Need help with that.

@isuruf

This comment has been minimized.

Copy link
Member

isuruf commented Mar 15, 2016

That's fine.
I'll create a new issue for improving could_extract_minus

isuruf added a commit that referenced this pull request Mar 15, 2016

@isuruf isuruf merged commit 7bbe523 into symengine:master Mar 15, 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 Mar 15, 2016

Thanks for the PR

@Sumith1896

This comment has been minimized.

Copy link
Member

Sumith1896 commented Mar 15, 2016

With merge commits, it's a bit more work to squash.
This should be fine for now. Thanks @CodeMaxx

@isuruf isuruf referenced this pull request Mar 16, 2016

Closed

Improve abs #834

0 of 2 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment