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: sage-6.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:

Status badges

Description (last modified by vdelecroix)

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 sage-devel https://groups.google.com/forum/#!topic/sage-devel/pFI9y3YZIQQ

Change History (12)

comment:1 Changed 7 years ago by vdelecroix

  • Branch set to u/vdelecroix/17443
  • Commit set to 697c759ec1dff3ab845b66beebc443aea5e9a1a5
  • Status changed from new to needs_review

New commits:

697c759trac #17443: fix abs(matrix)

comment:2 Changed 7 years ago by ncohen

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 vdelecroix

  • Description modified (diff)

No hurry. Discussion going on on sage-devel...

comment:4 follow-up: Changed 7 years ago by jdemeyer

  • 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 ncohen

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 git

  • Commit changed from 697c759ec1dff3ab845b66beebc443aea5e9a1a5 to f4ac40fe96cc610f0e7dd8bda02fc7049d783725

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

f4ac40ftrac #17443: deprecate abs + generic apply_map

comment:7 Changed 7 years ago by vdelecroix

  • Description modified (diff)
  • Status changed from needs_work to needs_review

comment:8 Changed 7 years ago by jdemeyer

  • Status changed from needs_review to needs_work

This should be a deprecation, not an error.

comment:9 Changed 7 years ago by git

  • Commit changed from f4ac40fe96cc610f0e7dd8bda02fc7049d783725 to e6a4de841482f86499d6ae16769e6a220624b3a6

Branch pushed to git repo; I updated commit sha1. New commits:

e6a4de8trac #17443: change TypeError to a deprecation

comment:10 Changed 7 years ago by vdelecroix

  • Status changed from needs_work to needs_review

All right, it is now a deprecation...

comment:11 Changed 7 years ago by ncohen

  • 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 vbraun

  • Branch changed from u/vdelecroix/17443 to e6a4de841482f86499d6ae16769e6a220624b3a6
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.