Skip to content
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

Matrix should not inherit from AlgebraElement #804

Closed
roed314 opened this issue Oct 3, 2007 · 21 comments
Closed

Matrix should not inherit from AlgebraElement #804

roed314 opened this issue Oct 3, 2007 · 21 comments

Comments

@roed314
Copy link
Contributor

roed314 commented Oct 3, 2007

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).

CC: @jasongrout @pjbruin

Component: linear algebra

Keywords: AlgebraElement, Matrix

Author: Peter Bruin

Branch/Commit: 05c7c30

Reviewer: Luis Felipe Tabera Alonso

Issue created by migration from https://trac.sagemath.org/ticket/804

@sagetrac-mabshoff
Copy link
Mannequin

sagetrac-mabshoff mannequin commented Dec 4, 2007

comment:1

Maybe now is a good time to do it?

Cheers,

Michael

@sagetrac-mabshoff sagetrac-mabshoff mannequin modified the milestones: sage-2.11, sage-2.9 Dec 4, 2007
@mwhansen
Copy link
Contributor

mwhansen commented Dec 5, 2007

comment:2

What are the reasons for the change in organization?

@robertwb
Copy link
Contributor

comment:4

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

@kcrisman
Copy link
Member

comment:5

What should it inherit from instead? This is a naive question, but perhaps someone with not much skill but much patience could fix this :)

@robertwb
Copy link
Contributor

comment:6

ModuleElement

@jdemeyer jdemeyer modified the milestones: sage-5.11, sage-5.12 Aug 13, 2013
@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.1, sage-6.2 Jan 30, 2014
@pjbruin
Copy link
Contributor

pjbruin commented Apr 2, 2014

Branch: u/pbruin/804-Matrix_inheritance

@pjbruin
Copy link
Contributor

pjbruin commented Apr 2, 2014

comment:11

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.

@pjbruin
Copy link
Contributor

pjbruin commented Apr 2, 2014

Author: Peter Bruin

@pjbruin
Copy link
Contributor

pjbruin commented Apr 2, 2014

Commit: 0fd3d31

@pjbruin

This comment has been minimized.

@kcrisman
Copy link
Member

kcrisman commented Apr 2, 2014

comment:12

Wow, nice necromancy! Dumb question - any other translations of tutorials have this bit which would need to be translated?

@pjbruin
Copy link
Contributor

pjbruin commented Apr 2, 2014

comment:13

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.

@kcrisman
Copy link
Member

kcrisman commented Apr 2, 2014

comment:14

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.

@videlec
Copy link
Contributor

videlec commented May 5, 2014

comment:15

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

@pjbruin
Copy link
Contributor

pjbruin commented May 5, 2014

comment:16

Replying to @videlec:

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...

@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin removed this from the sage-6.2 milestone May 6, 2014
@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin added this to the sage-6.3 milestone May 6, 2014
@pjbruin
Copy link
Contributor

pjbruin commented May 14, 2014

comment:18
sage -t --long src/sage/structure/element.pyx  # 2 doctests failed

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 14, 2014

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

05c7c30Trac 804: fix type error

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 14, 2014

Changed commit from 0fd3d31 to 05c7c30

@lftabera
Copy link
Contributor

Reviewer: Luis Felipe Tabera Alonso

@lftabera
Copy link
Contributor

comment:21

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.

@vbraun
Copy link
Member

vbraun commented Jul 19, 2014

Changed branch from u/pbruin/804-Matrix_inheritance to 05c7c30

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

No branches or pull requests