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

[with patch+review] polynomial/fraction field __hash__ re-write #1181

Closed
sagetrac-jbmohler mannequin opened this issue Nov 15, 2007 · 5 comments
Closed

[with patch+review] polynomial/fraction field __hash__ re-write #1181

sagetrac-jbmohler mannequin opened this issue Nov 15, 2007 · 5 comments

Comments

@sagetrac-jbmohler
Copy link
Mannequin

sagetrac-jbmohler mannequin commented Nov 15, 2007

The attached patch has a number of goals.

  1. Allow shared members of ZZ and ZZ[x] to be used interchangeably in a dictionary.
  2. Replace ZZ in !SAGE Notebook leaves dead processes on OS X #1 by QQ, IntegerModRing(p) and probably a bunch of other things.
  3. Replace ZZ[x] in !SAGE Notebook leaves dead processes on OS X #1 by ZZ[x,y,...]
  4. Allow shared members of FractionFields and their base rings to be used interchangeably in a dictionary.
  5. Make the __hash__ function faster in polynomial rings

Goal !#5 was achieved very nicely for univariate poly rings, but __hash__ in QQ[x,y] was extremely fast in the original code. Unfortunately, that very fast __hash__ violated all good things of the goals above. It also wasn't asymptotically fast (think huge numerical coefficients).

The goals 1-4 above result in a fix for the subs method:

sage: R.<x,y>=ZZ[]
sage: (x/y).subs({x:1})
1/y  # produced x/y in the old version

A bad thing about this patch is that the results of hash(x) changing reorders the output of things that come from a dictionary. There is a number of doc-tests output changes which are only a matter of order in the list. I think this is probably bad style in doc-tests and in real life. At least some of those outputs have a very natural order (to the human mind if not mathematically).

The attached patch entirely supersedes #1075 and I'm going to go close it right now.

Component: basic arithmetic

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

@sagetrac-jbmohler sagetrac-jbmohler mannequin added this to the sage-2.8.15 milestone Nov 15, 2007
@sagetrac-jbmohler
Copy link
Mannequin Author

sagetrac-jbmohler mannequin commented Nov 16, 2007

comment:1

Some benchmarks:

Original:

sage: R.<x>=ZZ[]
sage: one=R(1)
sage: f=x^2+2*x+1
sage: timeit hash(one)
1000000 loops, best of 3: 1.03 µs per loop
sage: timeit hash(f)
100000 loops, best of 3: 3.53 µs per loop
sage: timeit hash(x)
100000 loops, best of 3: 2.91 µs per loop
sage: R.<x,y>=QQ[]
sage: one=R(1)
sage: f=x^2+2*y+1
sage: timeit hash(one)
1000000 loops, best of 3: 931 ns per loop
sage: timeit hash(f)
1000000 loops, best of 3: 1.86 µs per loop
sage: timeit hash(x)
1000000 loops, best of 3: 486 ns per loop

Patched:

sage: # first the success
sage: R.<x>=ZZ[]
sage: one=R(1)
sage: f=x^2+2*x+1
sage: timeit hash(one)
1000000 loops, best of 3: 1.04 µs per loop
sage: timeit hash(f)
1000000 loops, best of 3: 1.98 µs per loop
sage: timeit hash(x)
1000000 loops, best of 3: 1.4 µs per loop
sage: # second: the huge slowdown
sage: R.<x,y>=QQ[]
sage: one=R(1)
sage: f=x^2+2*y+1
sage: timeit hash(one)
100000 loops, best of 3: 2.88 µs per loop
sage: timeit hash(f)
100000 loops, best of 3: 6.31 µs per loop
sage: timeit hash(x)
100000 loops, best of 3: 3.18 µs per loop

I believe (but haven't verified too much) that the big problem is converting coefficients to the base ring from the singular poly. This should probably be optimized for a whole bunch of reasons other than hashing, which will carry over to hashing.

@sagetrac-jbmohler
Copy link
Mannequin Author

sagetrac-jbmohler mannequin commented Nov 16, 2007

the patch

@sagetrac-jbmohler
Copy link
Mannequin Author

sagetrac-jbmohler mannequin commented Nov 16, 2007

comment:2

Attachment: hash-patch-2_8_12.hg.gz

@robertwb
Copy link
Contributor

robertwb commented Dec 2, 2007

comment:3

I've looked at the code and tried it out and it is a great idea that works well. Despite multivariate hashes slowing down, they are still really quite fast, and the properties listed are much more important.

@robertwb robertwb changed the title polynomial/fraction field __hash__ re-write [with patch+review] polynomial/fraction field __hash__ re-write Dec 2, 2007
@sagetrac-mabshoff
Copy link
Mannequin

sagetrac-mabshoff mannequin commented Dec 2, 2007

comment:4

Merged in 2.8.15.rc0.

@sagetrac-mabshoff sagetrac-mabshoff mannequin closed this as completed Dec 2, 2007
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

1 participant