Opened 14 years ago
Closed 7 years ago
#804 closed defect (fixed)
Matrix should not inherit from AlgebraElement
Reported by:  roed  Owned by:  was 

Priority:  minor  Milestone:  sage6.3 
Component:  linear algebra  Keywords:  AlgebraElement, Matrix 
Cc:  jason, pbruin  Merged in:  
Authors:  Peter Bruin  Reviewers:  Luis Felipe Tabera Alonso 
Report Upstream:  N/A  Work issues:  
Branch:  05c7c30 (Commits, GitHub, GitLab)  Commit:  05c7c30f33fccdf9fa159ce6fdab0c91f8393225 
Dependencies:  Stopgaps: 
Description (last modified by )
This ticket makes Matrix
only inherit from ModuleElement
.
Warning: it will cause practically all Cython files to be rebuilt.
See also #15215 (duplicate of this ticket).
Change History (22)
comment:1 Changed 14 years ago by
 Milestone changed from sage2.11 to sage2.9
comment:2 Changed 14 years ago by
What are the reasons for the change in organization?
comment:3 Changed 14 years ago by
 Component changed from algebraic geometry to linear algebra
comment:4 Changed 13 years ago by
The reason for the change is that not all matrices are Algebra Elements.
comment:5 Changed 12 years ago by
 Report Upstream set to N/A
What should it inherit from instead? This is a naive question, but perhaps someone with not much skill but much patience could fix this :)
comment:6 Changed 12 years ago by
comment:7 Changed 11 years ago by
 Cc jason added
comment:8 Changed 8 years ago by
 Milestone changed from sage5.11 to sage5.12
comment:9 Changed 7 years ago by
 Milestone changed from sage6.1 to sage6.2
comment:10 Changed 7 years ago by
 Cc pbruin added
comment:11 Changed 7 years ago by
 Branch set to u/pbruin/804Matrix_inheritance
 Commit set to 0fd3d318a230b2865cf331c2c696c9d05ceba487
 Description modified (diff)
 Status changed from new to needs_review
Given that this ticket has been open for more than seven years, it turned out to be surprisingly straightforward. There is one small simplification: Matrix
used to have two identical methods _mul_()
and __mul__()
, now it only needs __mul__()
. On the other hand, new (but very straightforward) __pow__()
and __div__()
methods were required.
comment:12 followup: ↓ 13 Changed 7 years ago by
Wow, nice necromancy! Dumb question  any other translations of tutorials have this bit which would need to be translated?
comment:13 in reply to: ↑ 12 ; followup: ↓ 14 Changed 7 years ago by
Replying to kcrisman:
any other translations of tutorials have this bit which would need to be translated?
It seems not; grep RingElement src/doc/*/tutorial/*
only finds the English and French ones.
comment:14 in reply to: ↑ 13 Changed 7 years ago by
any other translations of tutorials have this bit which would need to be translated?
It seems not;
grep RingElement src/doc/*/tutorial/*
only finds the English and French ones.
Interesting  apparently that must have been added after the other translations were made.
comment:15 followup: ↓ 16 Changed 7 years ago by
comment:16 in reply to: ↑ 15 Changed 7 years ago by
Replying to vdelecroix:
At first glance, I thought this ticket would help to build tropical matrices as discussed in #14507... but no, the "base ring" for tropical matrix is a semiring and not a ring (no requirement of an additive inverse). Am I right?
I think you are. Everywhere in Sage, matrices and vectors are assumed to have coefficients in some base ring. This would probably be much harder to change than the inheritance issue addressed in this ticket. If enough people want tropical matrices, then it seems we need new classes Matrix_semiring
and Vector_semiring
, possibly inheriting from some ModuleElement_semiring
...
comment:17 Changed 7 years ago by
 Milestone changed from sage6.2 to sage6.3
comment:18 Changed 7 years ago by
 Status changed from needs_review to needs_work
sage t long src/sage/structure/element.pyx # 2 doctests failed
comment:19 Changed 7 years ago by
 Commit changed from 0fd3d318a230b2865cf331c2c696c9d05ceba487 to 05c7c30f33fccdf9fa159ce6fdab0c91f8393225
Branch pushed to git repo; I updated commit sha1. New commits:
05c7c30  Trac 804: fix type error

comment:20 Changed 7 years ago by
 Status changed from needs_work to needs_review
comment:21 Changed 7 years ago by
 Reviewers set to Luis Felipe Tabera Alonso
 Status changed from needs_review to positive_review
Looks good to me. I also think that tropical matrices should have their own classes.
My copy did not compile against 6.3.beta3 but was probably more an issue with the beta since it failed before attempting to build the sage library. With 6.3.beta5 it works like a charm.
comment:22 Changed 7 years ago by
 Branch changed from u/pbruin/804Matrix_inheritance to 05c7c30f33fccdf9fa159ce6fdab0c91f8393225
 Resolution set to fixed
 Status changed from positive_review to closed
Maybe now is a good time to do it?
Cheers,
Michael