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

Improve matrix_modn_sparse methods for converting to dense and taking submatrices #10734

Open
tornaria opened this issue Feb 3, 2011 · 26 comments

Comments

@tornaria
Copy link
Contributor

tornaria commented Feb 3, 2011

Here I'll post some improvements that are needed for #10733 but are of independent interest.

Depends on #17837

Component: linear algebra

Author: Gonzalo Tornaria

Branch/Commit: public/matrix/improve_modn_sparse_methods-10734 @ d3f8b5f

Reviewer: William Stein, Travis Scrimshaw

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

@williamstein
Copy link
Contributor

comment:2

This ticket introduces a bunch of new functions, but doesn't include any documentation or doctests for them.

@tornaria
Copy link
Contributor Author

comment:3

The new patch adds documentation to all the functions created in the first patch.

@williamstein
Copy link
Contributor

comment:4

Great!

@jdemeyer
Copy link

Reviewer: William Stein

@jdemeyer jdemeyer added this to the sage-4.7 milestone Mar 25, 2011
@jdemeyer
Copy link

comment:6

Patches should be created using hg export tip and not hg diff or any other method. Also make sure you have a file $HOME/.hgrc as explained in the developer's manual.

@tornaria
Copy link
Contributor Author

comment:7

Replying to @jdemeyer:

Patches should be created using hg export tip and not hg diff or any other method. Also make sure you have a file $HOME/.hgrc as explained in the developer's manual.

Is it just this

# User Gonzalo Tornaria <[email protected]>

what the patches are missing?

This is a misfeature of mercurial (queues), because it doesn't set the user by default in the patches. Very silly.

Do you need me to reupload all the patches again, or can you just use

-u "Gonzalo Tornaria <[email protected]>"

as an option when you run hg import? Reuploading 7 files to trac gets boring (is there an easier way to a batch upload to trac? from the command line?).

[as a general comment; I'd prefer if the requirements are stated in terms of what the patches need to include, rather than dictating how one must use mercurial --
I find mercurial inconvenient to use without using queues, because the history cannot be rewritten and I don't know how to effectively use branches -- I've been spoiled by git, I guess]

@tornaria
Copy link
Contributor Author

Attachment: trac_10734.dense_matrix_and_submatrix.gz

For matrix_modn_sparse: implement optimized versions of submatrix(), dense_matrix(), add methods dense_submatrix() and set_block_unsafe()

@tornaria
Copy link
Contributor Author

Attachment: trac_10734.02-documentation.gz

Add documentation for the functions added in #10734

@tornaria
Copy link
Contributor Author

refactor code to support p=2 case (part 1)

@tornaria
Copy link
Contributor Author

comment:8

Attachment: trac_10734.03-referee_refactor_to_support_p2.patch.gz

I've reuploaded the two patches (hopefully) with the right format (username and date).

Also, I moved part of the referee patch in #10733 here, otherwise some doctests would fail until that ticket is merged.

There's no new code at all, so I assume the positive review is still valid.

@tornaria
Copy link
Contributor Author

comment:9

apply trac_10734.dense_matrix_and_submatrix, trac_10734.02-documentation, trac_10734.03-referee_refactor_to_support_p2.patch

@fchapoton
Copy link
Contributor

comment:10

Maybe you should add .patch at the end of the names of your patches. Otherwise they are not recognised as such, it seems.

@loefflerd
Copy link
Mannequin

loefflerd mannequin commented Mar 12, 2012

Apply only this patch. Patch against 5.0.beta7

@loefflerd
Copy link
Mannequin

loefflerd mannequin commented Mar 12, 2012

comment:11

Attachment: trac_10734-dense_matrix_and_submatrix-folded.patch.gz

I qfolded Gonzalo's patches into a single patch, and made sure it has ".patch" at the end of its name, as Frederic suggests. But with this (or, equivalently, Gonzalo's original three patches) installed I get some doctest failures in sage/matrix/matrix_modn_sparse -- any ideas what's causing that?

@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.2, sage-6.3 May 6, 2014
@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.3, sage-6.4 Aug 10, 2014
@tscrim
Copy link
Collaborator

tscrim commented Sep 14, 2014

comment:17

Git branch and also has the fixes for the sparse conversion by making the matrix_modn_dense_template inherit from matrix_mod_dense


New commits:

c0a74b9#10734: Matrix_modn_sparse speed improvements
8bc6935Made modn dense template inherit from mod_dense.

@tscrim
Copy link
Collaborator

tscrim commented Sep 14, 2014

Changed reviewer from William Stein to William Stein, Travis Scrimshaw

@tscrim
Copy link
Collaborator

tscrim commented Sep 14, 2014

@tscrim
Copy link
Collaborator

tscrim commented Sep 14, 2014

Commit: 8bc6935

@fchapoton
Copy link
Contributor

comment:18

needs rebase

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 7, 2014

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

d3f8b5fMerge branch 'public/matrix/improve_modn_sparse_methods-10734' of trac.sagemath.org:sage into public/matrix/improve_modn_sparse_methods-10734

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 7, 2014

Changed commit from 8bc6935 to d3f8b5f

@tscrim
Copy link
Collaborator

tscrim commented Nov 7, 2014

comment:20

Rebased to 6.4.rc1.

@videlec
Copy link
Contributor

videlec commented Jan 25, 2015

comment:21

Hello,

What is the purpose of introducing the class Matrix_mod_dense?

Vincent

@videlec
Copy link
Contributor

videlec commented Feb 23, 2015

comment:22

Ticket #17837 proposes to remove matrix_modn_dense (it is unused since a long time and much slower than matrix_modn_dense_float or matrix_modn_dense_double using LinBox). Is there anybody working on this ticket that can say something about it? Otherwise I will put #17837 in positive review (and consequently this one in needs work).

Vincent

@jdemeyer
Copy link

Dependencies: #17837

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

7 participants