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

Remove matrix_modn_dense.pyx #17837

Closed
jdemeyer opened this issue Feb 23, 2015 · 23 comments
Closed

Remove matrix_modn_dense.pyx #17837

jdemeyer opened this issue Feb 23, 2015 · 23 comments

Comments

@jdemeyer
Copy link

This has been unused since #4260, remove it.

Component: linear algebra

Author: Jeroen Demeyer

Branch: 766f1a5

Reviewer: Vincent Delecroix

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

@jdemeyer jdemeyer added this to the sage-6.6 milestone Feb 23, 2015
@jdemeyer
Copy link
Author

Author: André Apitzsch

@jdemeyer
Copy link
Author

Branch: u/jdemeyer/ticket/17837

@jdemeyer
Copy link
Author

comment:3

Do we even need the determinant method in the first place? Why not rely on inheritance?


New commits:

e768168fix some cython warnings

@jdemeyer
Copy link
Author

Commit: e768168

@jdemeyer

This comment has been minimized.

@jdemeyer

This comment has been minimized.

@jdemeyer
Copy link
Author

Changed author from André Apitzsch to Jeroen Demeyer

@jdemeyer jdemeyer changed the title Remove redundancies from matrix_modn_dense.pyx Remove matrix_modn_dense.pyx Feb 23, 2015
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 23, 2015

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

74d2c66Remove matrix_modn_dense

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 23, 2015

Changed commit from e768168 to 74d2c66

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 23, 2015

Changed commit from 74d2c66 to a6112ce

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 23, 2015

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

a6112ceRemove matrix_modn_dense

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 23, 2015

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

766f1a5Remove matrix_modn_dense

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 23, 2015

Changed commit from a6112ce to 766f1a5

@videlec
Copy link
Contributor

videlec commented Feb 23, 2015

Reviewer: Vincent Delecroix

@videlec
Copy link
Contributor

videlec commented Feb 23, 2015

comment:11

Instead of

        if not (isinstance(b, Matrix_modn_dense_float) or
                isinstance(b, Matrix_modn_dense_double)):

you might have used

        if not isinstance(b, (Matrix_modn_dense_float, Matrix_modn_dense_double)):

which is handled in the exact same way by cython.

I will try to use the nice unpickle_override for #17824.

@videlec
Copy link
Contributor

videlec commented Feb 23, 2015

comment:12

Overlaps with #10734.

@vbraun
Copy link
Member

vbraun commented Feb 24, 2015

Changed branch from u/jdemeyer/ticket/17837 to 766f1a5

@videlec
Copy link
Contributor

videlec commented Feb 24, 2015

Changed commit from 766f1a5 to none

@videlec
Copy link
Contributor

videlec commented Feb 24, 2015

comment:14

What is this transition "needs work -> closed"!?

@jdemeyer
Copy link
Author

comment:15

On the other hand, I don't understand why the mere existence of ticket #10734 would be a reason for needs_work.

@jdemeyer
Copy link
Author

comment:16

Replying to @videlec:

Instead of

        if not (isinstance(b, Matrix_modn_dense_float) or
                isinstance(b, Matrix_modn_dense_double)):

you might have used

        if not isinstance(b, (Matrix_modn_dense_float, Matrix_modn_dense_double)):

which is handled in the exact same way by cython.

Obviously, I just didn't think of that. It doesn't really matter anyway...

@videlec
Copy link
Contributor

videlec commented Feb 24, 2015

comment:17

Replying to @jdemeyer:

On the other hand, I don't understand why the mere existence of ticket #10734 would be a reason for needs_work.

Because there is a large amount of work on ticket #10734 that uses matrix_modn_dense. Before throwing away this work, it would have been nice to wait for the current status of this work. I am pretty sure that everything can be achieved with the LinBox classes anyway.

@jdemeyer
Copy link
Author

comment:18

Replying to @videlec:

Because there is a large amount of work on ticket #10734 that uses matrix_modn_dense.

But with or without #17837, matrix_modn_dense is deprecated and no longer used. So if #10734 really uses Matrix_modn_dense, it should be changed anyway.

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

3 participants