Opened 12 years ago
Closed 12 years ago
#9590 closed defect (fixed)
Doctest failures in cone.py and toric_lattice_element.pyx on 32bit Linux
Reported by:  mpatel  Owned by:  mhampton 

Priority:  blocker  Milestone:  sage4.5.2 
Component:  geometry  Keywords:  
Cc:  cremona, davidloeffler, leif, novoselt, vbraun  Merged in:  sage4.5.2.alpha1 
Authors:  Andrey Novoseltsev  Reviewers:  Leif Leonhardy 
Report Upstream:  N/A  Work issues:  
Branch:  Commit:  
Dependencies:  Stopgaps: 
Description
Seen by John Cremona and Leif Leonhardy on 32bit SUSE and 32bit Ubuntu, respectively:
sage t long "devel/sage/sage/geometry/toric_lattice_element.pyx" ********************************************************************** File "/local/jec/sage4.5.2.alpha0/devel/sage/sage/geometry/toric_lattice_element.pyx", line 235: sage: n.set_immutable() Expected: 2528502973977326415 Got nothing sage t long "devel/sage/sage/geometry/cone.py" ********************************************************************** File "/local/jec/sage4.5.2.alpha0/devel/sage/sage/geometry/cone.py", line 559: sage: c = Cone([(1,0), (0,1)]) Expected: 4372618627376133801 Got nothing
Attachments (1)
Change History (11)
Changed 12 years ago by
comment:1 Changed 12 years ago by
 Status changed from new to needs_review
comment:2 followup: ↓ 3 Changed 12 years ago by
leif@californication:~/sage4.5.2.alpha0j6/devel/sage9590$ ../../sage t long sage/geometry/ sage t long "devel/sage9590/sage/geometry/fan.py" [10.9 s] sage t long "devel/sage9590/sage/geometry/polytope.py" [3.0 s] sage t long "devel/sage9590/sage/geometry/__init__.py" [0.1 s] sage t long "devel/sage9590/sage/geometry/all.py" [0.1 s] sage t long "devel/sage9590/sage/geometry/cone.py" [7.9 s] sage t long "devel/sage9590/sage/geometry/lattice_polytope.py" [13.0 s] sage t long "devel/sage9590/sage/geometry/polyhedra.py" [152.0 s] sage t long "devel/sage9590/sage/geometry/toric_lattice.py" [3.1 s] sage t long "devel/sage9590/sage/geometry/toric_lattice_element.pyx" [3.1 s]  All tests passed! Total time for all tests: 193.4 seconds leif@californication:~/sage4.5.2.alpha0j6/devel/sage9590$ hg log  head n 11 changeset: 14742:ebb1e171e138 tag: tip user: Andrey Novoseltsev <novoselt@gmail.com> date: Fri Jul 23 23:09:59 2010 0600 summary: Trac 9590: Doctest failures in cone and toric_lattice_element. changeset: 14741:af5f40a73eda user: Mitesh Patel <qed777@gmail.com> date: Wed Jul 21 20:13:55 2010 0700 summary: 4.5.2.alpha0
(This is on Ubuntu 9.04 x86, Pentium 4 Prescott, gcc 4.3.3.)
So I can give Andrey's patch a positive review.
We could have added the hash codes for 32bit systems, too, instead.
comment:3 in reply to: ↑ 2 ; followup: ↓ 4 Changed 12 years ago by
Replying to leif:
We could have added the hash codes for 32bit systems, too, instead.
We could, but I don't know how to get those values for 32bit systems on 64bit ones and as I understand those numbers don't have any sense anyway and can potentially change. So these doctests just check that hash
can be used and it returns the same value. If adding 32bit hashes would be better, please add them.
I find it very peculiar how slow is polyhedra.py
in this test. On sage.math most tests are a little bit faster, but this one takes only 41 second! On my quite old Mobile AMD Athlon 64 3000+ (1.8GHz) results are closer to those above, but polyhedra.py
still tests only in 74 seconds! Is it just the difference between 32 and 64 bits?..
comment:4 in reply to: ↑ 3 Changed 12 years ago by
Replying to novoselt:
I find it very peculiar how slow is
polyhedra.py
in this test. On sage.math most tests are a little bit faster, but this one takes only 41 second! On my quite old Mobile AMD Athlon 64 3000+ (1.8GHz) results are closer to those above, butpolyhedra.py
still tests only in 74 seconds! Is it just the difference between 32 and 64 bits?..
This is on an otherwise idle Core2 (64bit):
leif@quadriga:~/sage4.5.2.alpha0$ ./sage t long devel/sage/sage/geometry/ sage t long "devel/sage/sage/geometry/__init__.py" [0.0 s] sage t long "devel/sage/sage/geometry/toric_lattice_element.pyx" [1.0 s] sage t long "devel/sage/sage/geometry/toric_lattice.py" [1.0 s] sage t long "devel/sage/sage/geometry/polytope.py" [1.0 s] sage t long "devel/sage/sage/geometry/lattice_polytope.py" [5.1 s] sage t long "devel/sage/sage/geometry/fan.py" [4.7 s] sage t long "devel/sage/sage/geometry/cone.py" [3.3 s] sage t long "devel/sage/sage/geometry/all.py" [0.0 s] sage t long "devel/sage/sage/geometry/polyhedra.py" [58.7 s]  All tests passed! Total time for all tests: 74.9 seconds
comment:5 Changed 12 years ago by
Just for the record: I've just tested the patch on that system, too. All tests in sage/geometry
now take 0.4s less in total... ;)
(The timings per file vary by sometimes more than 0.1s even on an idling machine.)
comment:6 Changed 12 years ago by
If anyone wants this to be fixed in a different way, here are two alternatives in a single patch:

sage/geometry/cone.py
diff git a/sage/geometry/cone.py b/sage/geometry/cone.py
a b 557 557 TESTS:: 558 558 559 559 sage: c = Cone([(1,0), (0,1)]) 560 sage: hash(c) # 64bit 561 4372618627376133801 560 sage: hash(c) 561 1996666537 # 32bit 562 4372618627376133801 # 64bit 563 sage: c2 = Cone([(1,0), (0,1)]) 564 sage: hash(c) == hash(c2) 565 True 562 566 """ 563 567 if "_hash" not in self.__dict__: 564 568 self._hash = hash(self._rays) 
sage/geometry/toric_lattice_element.pyx
diff git a/sage/geometry/toric_lattice_element.pyx b/sage/geometry/toric_lattice_element.pyx
a b 233 233 ... 234 234 TypeError: mutable vectors are unhashable 235 235 sage: n.set_immutable() 236 sage: hash(n) # 64bit 237 2528502973977326415 236 sage: hash(n) 237 378539185 # 32bit 238 2528502973977326415 # 64bit 239 sage: n2 = N(1,2,3) 240 sage: n2.set_immutable() 241 sage: hash(n) == hash(n2) 242 True 238 243 """ 239 244 return Vector_integer_dense.__hash__(self)
(This is against vanilla alpha0.)
Otherwise (Seattle, wake up!) I'll set this to "positive review" (as is), s.t. it can be merged into alpha1.
comment:7 Changed 12 years ago by
 Reviewers set to Leif Leonhardy
 Status changed from needs_review to positive_review
Ok, since nobody complained [in time], I'll set this to "positive review".
Mitesh, please merge... ;)
comment:8 Changed 12 years ago by
I also see the failures in the description on t2 with 4.5.2.alpha0 + #8059 (I ignored the patch rejects). But the patch above fixes them.
comment:9 Changed 12 years ago by
I thought that maybe Leif's suggestions would be preferred, but Carl Witty said (https://groups.google.com/group/sagedevel/browse_thread/thread/9a0f357c8ec9bbd):
Hmm... looks like the current state of affairs is a mess. Looking through the 'def __hash__' grep hits in sage/rings, there are quite a few of each of the following: 1) no doctest at all 2) provide both 32bit and 64bit doctests 3) define your hash function to produce a 32bit output that's the same on 32bit and 64bit systems; doctest an instance of that output 4) doctest hash value equality without ever showing a doctest output plus one instance where the hash output is marked "# random". So whatever you do with this particular patch, it won't make things much worse :)
So, I'll merge this, and maybe we'll figure out a better way to test hashing later.
comment:10 Changed 12 years ago by
 Merged in set to sage4.5.2.alpha1
 Resolution set to fixed
 Status changed from positive_review to closed
This should work, please test it on a 32bit system as I don't have one set up.