Opened 7 years ago
Closed 7 years ago
#17443 closed defect (fixed)
abs(matrix) should not be a shortcut for det
Reported by:  vdelecroix  Owned by:  

Priority:  major  Milestone:  sage6.5 
Component:  linear algebra  Keywords:  
Cc:  Merged in:  
Authors:  Vincent Delecroix  Reviewers:  Nathann Cohen 
Report Upstream:  N/A  Work issues:  
Branch:  e6a4de8 (Commits, GitHub, GitLab)  Commit:  e6a4de841482f86499d6ae16769e6a220624b3a6 
Dependencies:  Stopgaps: 
Description (last modified by )
We have currently
sage: M = matrix([[1]]) sage: abs(M) 1
Because matrix.__abs__
is a shortcut for determinant!!I
n scipy, __abs__
applies the absolute value to each coefficient. But it is not likely what we want to do in Sage. Instead we raise a TypeError
and inform the user about matrix.norm(1)
and matrix.apply_map(abs)
.
Related discussion on sagedevel https://groups.google.com/forum/#!topic/sagedevel/pFI9y3YZIQQ
Change History (12)
comment:1 Changed 7 years ago by
 Branch set to u/vdelecroix/17443
 Commit set to 697c759ec1dff3ab845b66beebc443aea5e9a1a5
 Status changed from new to needs_review
comment:2 Changed 7 years ago by
Crazy O_o
I will run the tests and see if it breaks anything.
Why do you make "abs" an alias for __abs__
? If you believe that it is worth adding a function (given that abs(M)
works I do not understand why it would be), can you make it .absolute_value
? We try to not have full names for methods.
Nathann
comment:3 Changed 7 years ago by
 Description modified (diff)
No hurry. Discussion going on on sagedevel...
comment:4 followup: ↓ 5 Changed 7 years ago by
 Status changed from needs_review to needs_work
The discussions seem to indicate a preference for deprecating __abs__
.
comment:5 in reply to: ↑ 4 Changed 7 years ago by
The discussions seem to indicate a preference for deprecating
__abs__
.
Yesyes, this seems to be the wisest thing to do indeed.
Nathann
comment:6 Changed 7 years ago by
 Commit changed from 697c759ec1dff3ab845b66beebc443aea5e9a1a5 to f4ac40fe96cc610f0e7dd8bda02fc7049d783725
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
f4ac40f  trac #17443: deprecate abs + generic apply_map

comment:7 Changed 7 years ago by
 Description modified (diff)
 Status changed from needs_work to needs_review
comment:8 Changed 7 years ago by
 Status changed from needs_review to needs_work
This should be a deprecation, not an error.
comment:9 Changed 7 years ago by
 Commit changed from f4ac40fe96cc610f0e7dd8bda02fc7049d783725 to e6a4de841482f86499d6ae16769e6a220624b3a6
Branch pushed to git repo; I updated commit sha1. New commits:
e6a4de8  trac #17443: change TypeError to a deprecation

comment:10 Changed 7 years ago by
 Status changed from needs_work to needs_review
All right, it is now a deprecation...
comment:11 Changed 7 years ago by
 Reviewers set to Nathann Cohen
 Status changed from needs_review to positive_review
Passes all tests, so positive review.
Do you know of a tool to detect that you only "moved" the code of those two big functions ?
I found no way to do this, so in order to check your patch I moved what you wrote in matrix2 back to matrix_dense, only to see as it was detected as "leaving those function as they were" when merged with your patch.
Nathann
comment:12 Changed 7 years ago by
 Branch changed from u/vdelecroix/17443 to e6a4de841482f86499d6ae16769e6a220624b3a6
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
trac #17443: fix abs(matrix)