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

faster hashs for Matrix_mod2_dense #3724

Closed
malb opened this issue Jul 25, 2008 · 15 comments
Closed

faster hashs for Matrix_mod2_dense #3724

malb opened this issue Jul 25, 2008 · 15 comments

Comments

@malb
Copy link
Member

malb commented Jul 25, 2008

Simon King requested faster hashing for matrices over GF(2). This patch implements it, but depends on #3324 and an updated M4RI.

CC: @simon-king-jena

Component: linear algebra

Keywords: m4ri, hash, matrix

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

@malb malb added this to the sage-3.1.2 milestone Jul 25, 2008
@malb malb self-assigned this Jul 25, 2008
@malb
Copy link
Member Author

malb commented Jul 25, 2008

implements faster hashing

@malb
Copy link
Member Author

malb commented Aug 6, 2008

comment:1

Attachment: m4ri_hash.patch.gz

it seems, the patch is independent of the PNG fix.

@malb malb changed the title [depends on other ticket] faster hashs for Matrix_mod2_dense faster hashs for Matrix_mod2_dense Aug 6, 2008
@malb
Copy link
Member Author

malb commented Aug 24, 2008

comment:2

Simon, can you review my patch?

@simon-king-jena
Copy link
Member

comment:3

The patch applies to sage-3.1.1

Consider a little test:

sage: M=MatrixSpace(GF(2),10000,10000).random_element()
sage: M.set_immutable()
sage: time M.__hash__()

Without the patch, i had to interrupt sage after few minutes since it ate pretty much of my computer's memory.

With the patch, we get

sage: time M.__hash__()
CPU times: user 0.00 s, sys: 0.00 s, total: 0.00 s
Wall time: 0.00 s
-2570088162955604276

Well done, Martin! I give a positive review.

The patch contains a doc-test. One problem for me: Typing M.__hash__?, i don't see them. What is the user supposed to do in order to see the doc-string of the respective hash method?

I know that the following should be another ticket. But here are two items on my wish list:

  1. Can similar things be done with the hash of matrices over GF(p), p>2?
  2. Please also improve pickling of matrices! Example:
sage: M=MatrixSpace(GF(2),1000,1000).random_element()
sage: timeit('x=loads(dumps(M))')
5 loops, best of 3: 2.33 s per loop

which is somehow slowish.

@sagetrac-mabshoff
Copy link
Mannequin

sagetrac-mabshoff mannequin commented Aug 26, 2008

comment:4

Simon,

please open a thread on sage-devel. I assume the discussion will conclude that speed up is warranted and then we can open tickets for the new issues.

Cheers,

Michael

@sagetrac-mabshoff
Copy link
Mannequin

sagetrac-mabshoff mannequin commented Aug 26, 2008

comment:5

Merged in Sage 3.1.2.alpha1

@sagetrac-mabshoff sagetrac-mabshoff mannequin closed this as completed Aug 26, 2008
@malb
Copy link
Member Author

malb commented Aug 26, 2008

comment:6

Replying to @simon-king-jena:

The patch contains a doc-test. One problem for me: Typing M.__hash__?, i don't see them. What is the user supposed to do in order to see the doc-string of the respective hash method?

That could be a bug for introspection (or however that thingy is called). Could you open a ticket?

  1. Can similar things be done with the hash of matrices over GF(p), p>2?

Yes it can, please open a Trac ticket and I'll do it as soon as I find some time.

  1. Please also improve pickling of matrices! Example:
sage: M=MatrixSpace(GF(2),1000,1000).random_element()
sage: timeit('x=loads(dumps(M))')
5 loops, best of 3: 2.33 s per loop

which is somehow slowish.

That is #3324 which is blocked by a problem on OSX 10.4 and libpng.

@simon-king-jena
Copy link
Member

comment:7

Sorry, but i think i should re-open the ticket:

sage: M = MatrixSpace(GF(2),10000,10000).random_element()
sage: M.is_mutable()
True
sage: M.__hash__()
354973654957594997

A mutable object is not allowed to have a hash, AFAIK.
On the other hand, the hash-value seems not to be cached:

sage: M[0,0]
1
sage: M[0,0]=0
sage: M.__hash__()
-8868398381897180811

So, the hash value has changed (i.e., was re-computed) by changing the matrix...

sage: N=copy(M)
sage: N.__hash__()
-8868398381897180811

... and has not changed by copying the matrix.

By consequence, it may be that everything is alright.

However, I re-open the ticket, because I think this should be addressed -- either by a new patch raising an exception when M is mutable, or by the assertion that "mutable objects have no hash" is a rule but no law, and that it is fine since the hash is not cached.

Cheers
Simon

@simon-king-jena simon-king-jena changed the title faster hashs for Matrix_mod2_dense [positive and negative review] faster hashs for Matrix_mod2_dense Aug 26, 2008
@malb
Copy link
Member Author

malb commented Aug 26, 2008

address review

@malb
Copy link
Member Author

malb commented Aug 26, 2008

comment:8

Attachment: m4ri_hash2.patch.gz

You're right, good catch. The attached patch addresses that issue.

@sagetrac-mabshoff
Copy link
Mannequin

sagetrac-mabshoff mannequin commented Aug 26, 2008

comment:9

m4ri_hash2.patch looks good to me. Positive review.

@sagetrac-mabshoff sagetrac-mabshoff mannequin changed the title [positive and negative review] faster hashs for Matrix_mod2_dense [positive] faster hashs for Matrix_mod2_dense Aug 26, 2008
@sagetrac-mabshoff
Copy link
Mannequin

sagetrac-mabshoff mannequin commented Aug 26, 2008

comment:10

Merged m4ri_hash2.patch in Sage 3.1.2.alpha1

@sagetrac-mabshoff sagetrac-mabshoff mannequin closed this as completed Aug 26, 2008
@malb
Copy link
Member Author

malb commented Aug 27, 2008

comment:11

The new hash-ing method does not obey Sage's hashing rules. See Robert's comment at #3956. Obeying this rule however would come with a considerable speed penalty compared to m4ri_hash.patch.

@malb malb changed the title [positive] faster hashs for Matrix_mod2_dense faster hashs for Matrix_mod2_dense Aug 27, 2008
@malb malb reopened this Aug 27, 2008
@sagetrac-mabshoff
Copy link
Mannequin

sagetrac-mabshoff mannequin commented Aug 27, 2008

comment:12

Martin,

can we move the latest issue to a new ticket? As is this ticket is getting rather messy, i.e. in HISTORY.txt as well as here.

Cheers,

Michael

@malb
Copy link
Member Author

malb commented Aug 27, 2008

comment:13

your wish is my command.

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

2 participants