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

Support for __abs__ in SymPy matrices #12686

Merged
merged 3 commits into from Jun 5, 2017

Conversation

Projects
None yet
2 participants
@bjodah
Member

bjodah commented May 29, 2017

I was surprised to see __abs__ being unsupported by matrices.
I think this should be quite uncontroversial, unless there is chance users would expect some kind
of matrix norm instead.

pinging @siefkenj for feedback.

@siefkenj

This comment has been minimized.

Show comment
Hide comment
@siefkenj

siefkenj May 29, 2017

Contributor

This indeed seems uncontroversial. Could you put __abs__ in alphabetical order with the other methods and add an _eval_abs method that it calls? That way, for instance SparseMatrix could optimize the call. Also, a docstring says """Entry-wise absolute value""" would eliminate confusion over whether it was a norm or not.

Contributor

siefkenj commented May 29, 2017

This indeed seems uncontroversial. Could you put __abs__ in alphabetical order with the other methods and add an _eval_abs method that it calls? That way, for instance SparseMatrix could optimize the call. Also, a docstring says """Entry-wise absolute value""" would eliminate confusion over whether it was a norm or not.

@bjodah

This comment has been minimized.

Show comment
Hide comment
@bjodah

bjodah May 30, 2017

Member

Sure, I named it _eval_Abs since it actually applies Abs and not abs on the elements (_eval_Abs is used elsewhere in the codebase), is that ok?

Member

bjodah commented May 30, 2017

Sure, I named it _eval_Abs since it actually applies Abs and not abs on the elements (_eval_Abs is used elsewhere in the codebase), is that ok?

def test_abs():
m = Matrix(1, 2, [-3, x])
n = Matrix(1, 2, [3, Abs(x)])
assert abs(m) == n

This comment has been minimized.

@siefkenj

siefkenj May 30, 2017

Contributor

could you add a test like this to test_commonmatrix.py in the ArithmeticOnlyMatrix section?

@siefkenj

siefkenj May 30, 2017

Contributor

could you add a test like this to test_commonmatrix.py in the ArithmeticOnlyMatrix section?

@siefkenj

This comment has been minimized.

Show comment
Hide comment
@siefkenj

siefkenj May 30, 2017

Contributor

Once the test gets added, this looks good. It occurs to me that abs for matrices and abs for points in the geometry module now differ, but I think this behavior is better.

Contributor

siefkenj commented May 30, 2017

Once the test gets added, this looks good. It occurs to me that abs for matrices and abs for points in the geometry module now differ, but I think this behavior is better.

@siefkenj

This comment has been minimized.

Show comment
Hide comment
@siefkenj

siefkenj Jun 5, 2017

Contributor

I think this is ready to merge.

Contributor

siefkenj commented Jun 5, 2017

I think this is ready to merge.

@siefkenj siefkenj referenced this pull request Jun 5, 2017

Merged

Matrix Cleanup 7 #12711

@bjodah bjodah merged commit 1e1d896 into sympy:master Jun 5, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bjodah

This comment has been minimized.

Show comment
Hide comment
@bjodah

bjodah Jun 5, 2017

Member

Thanks!

Member

bjodah commented Jun 5, 2017

Thanks!

@bjodah bjodah deleted the bjodah:matrix-abs branch Jun 5, 2017

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