Opened 13 years ago

Closed 6 years ago

#804 closed defect (fixed)

Matrix should not inherit from AlgebraElement

Reported by: roed Owned by: was
Priority: minor Milestone: sage-6.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) Commit: 05c7c30f33fccdf9fa159ce6fdab0c91f8393225
Dependencies: Stopgaps:

Description (last modified by pbruin)

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 13 years ago by mabshoff

  • Milestone changed from sage-2.11 to sage-2.9

Maybe now is a good time to do it?

Cheers,

Michael

comment:2 Changed 13 years ago by mhansen

What are the reasons for the change in organization?

comment:3 Changed 12 years ago by AlexGhitza

  • Component changed from algebraic geometry to linear algebra

comment:4 Changed 12 years ago by robertwb

The reason for the change is that not all matrices are Algebra Elements.

comment:5 Changed 11 years ago by kcrisman

  • 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 11 years ago by robertwb

comment:7 Changed 9 years ago by jason

  • Cc jason added

comment:8 Changed 7 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:9 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:10 Changed 6 years ago by pbruin

  • Cc pbruin added

comment:11 Changed 6 years ago by pbruin

  • Authors set to Peter Bruin
  • Branch set to u/pbruin/804-Matrix_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 follow-up: Changed 6 years ago by kcrisman

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 ; follow-up: Changed 6 years ago by pbruin

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 6 years ago by 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.

Interesting - apparently that must have been added after the other translations were made.

comment:15 follow-up: Changed 6 years ago by vdelecroix

Hi,

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?

Vincent

comment:16 in reply to: ↑ 15 Changed 6 years ago by pbruin

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 6 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:18 Changed 6 years ago by pbruin

  • Status changed from needs_review to needs_work
sage -t --long src/sage/structure/element.pyx  # 2 doctests failed

comment:19 Changed 6 years ago by git

  • Commit changed from 0fd3d318a230b2865cf331c2c696c9d05ceba487 to 05c7c30f33fccdf9fa159ce6fdab0c91f8393225

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

05c7c30Trac 804: fix type error

comment:20 Changed 6 years ago by pbruin

  • Status changed from needs_work to needs_review

comment:21 Changed 6 years ago by lftabera

  • 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 6 years ago by vbraun

  • Branch changed from u/pbruin/804-Matrix_inheritance to 05c7c30f33fccdf9fa159ce6fdab0c91f8393225
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.