On Mon, Apr 18, 2011 at 10:24 PM, William Stein <[email protected]> wrote: > On Mon, Apr 18, 2011 at 9:09 PM, Robert Bradshaw > <[email protected]> wrote: >> On Mon, Apr 18, 2011 at 4:13 PM, William Stein <[email protected]> wrote: >>> Hi, >>> >>> I was just explaining to a student in my Sage course how I had >>> stupidly defined a default __hash__ method for SageObject, which was >>> -- stupidly -- to just hash the string representation. This was of >>> course silly and dangerous since only immutable objects should have a >>> __hash__ method. I then of course pointed out that we must have >>> fixed this years ago! But.... amazingly we didn't: >>> >>> sage: S = SageObject(); S >>> <type 'sage.structure.sage_object.SageObject'> >>> sage: hash(S) >>> -7904861314369208036 >>> sage: S.__hash__?? >>> File: /sagenb/flask/sage-4.6.2/devel/sage/sage/structure/sage_object.pyx >>> Source Code (starting at line 156): >>> def __hash__(self): >>> return hash(self.__repr__()) >> >> By convention, almost all SageObjects are immutable, so it was >> probably easier to provide a default and override it for exceptions >> (matrices and vectors are all that come to mind) than implement it for >> every concrete subclass. >> - Robert > > The statement "... so it was probably easier to provide a default and > override it for exceptions (matrices and vectors are all that come to > mind) than implement it for every concrete subclass." is indeed why I > implemented __hash__ that way, long ago. However, I didn't do it > because by convention almost all SageObjects are immutable. In fact, > many of them were not immutable in the beginning, and later became > immutable when I learned the extent to which mutable hashable break > basic assumptions in Python (and rightfully so). > > I think it is worth revisiting this design decision. I did a > little poking, and in Python and Cython, a class by default has a > hash, which is the location in memory of the object, e.g., > > sage: class Foo: > ....: pass > ....: > sage: hash(f) > 140309021077656 > sage: id(f) > 140309021077656 > > So the main issue I pointed out above, namely that SageObject has a > __hash__ defined, is really a non-issue, since that's the standard > default convention in Python anyways. The real issue to consider is > whether using the string representation as a default is a good idea.
The advantage is that it (usually) solves the equal -> equal hash issue without requiring uniqueness fairly nicely. Arbitrarily breaking this might be dangerous and I'm wary that our doctests are insufficient to cover all corner cases where things could go badly and strangely wrong. Better to raise an error by default and then fix things. > The only real drawback is efficiency, since computing the string > representation of an object can be costly. So... > > Project: comment out the two lines in sage_object.pyx, run the Sage > test suite, observe failures [1], choose one, and write a better > (faster; more intelligent) __hash__ method for the objects that lead > to the failure. > > -- William > > [1] The files in devel/sage/sage that fail are: > > The following tests failed: > > sage -t devel/sage-main/sage/plot/plot3d/base.pyx # 1 doctests failed > sage -t devel/sage-main/sage/schemes/generic/toric_variety.py > # 4 doctests failed > sage -t > devel/sage-main/sage/rings/polynomial/multi_polynomial_ideal.py # 2 > doctests failed > sage -t > devel/sage-main/sage/schemes/elliptic_curves/heegner.py # 2 doctests > failed > sage -t devel/sage-main/sage/interfaces/maxima.py # 4 doctests failed > sage -t devel/sage-main/sage/categories/pushout.py # 3 doctests failed > sage -t > devel/sage-main/sage/schemes/elliptic_curves/ell_curve_isogeny.py # 8 > doctests failed > sage -t > devel/sage-main/sage/rings/polynomial/polynomial_element.pyx # 2 > doctests failed > sage -t devel/sage-main/sage/crypto/mq/mpolynomialsystem.py # > 1 doctests failed > sage -t > devel/sage-main/sage/rings/polynomial/multi_polynomial_sequence.py # 1 > doctests failed > sage -t devel/sage-main/sage/combinat/yang_baxter_graph.py # > 1 doctests failed > sage -t > devel/sage-main/sage/rings/polynomial/multi_polynomial_libsingular.pyx > # 3 doctests failed > sage -t > devel/sage-main/sage/rings/polynomial/multi_polynomial.pyx # 2 > doctests failed > sage -t devel/sage-main/sage/tests/startup.py # 1 doctests failed > sage -t devel/sage-main/sage/categories/functor.pyx # 1 doctests > failed > sage -t > devel/sage-main/sage/rings/polynomial/infinite_polynomial_ring.py # 6 > doctests failed > sage -t > devel/sage-main/sage/sets/disjoint_union_enumerated_sets.py # 10 > doctests failed > sage -t devel/sage-main/sage/schemes/plane_curves/curve.py # > 1 doctests failed > sage -t > devel/sage-main/sage/rings/polynomial/laurent_polynomial_ring.py # 4 > doctests failed > sage -t > devel/sage-main/sage/rings/polynomial/polynomial_ring_constructor.py # > 2 doctests failed > sage -t > devel/sage-main/sage/combinat/root_system/type_relabel.py # 2 doctests > failed > > -- > To post to this group, send an email to [email protected] > To unsubscribe from this group, send an email to > [email protected] > For more options, visit this group at > http://groups.google.com/group/sage-devel > URL: http://www.sagemath.org > -- To post to this group, send an email to [email protected] To unsubscribe from this group, send an email to [email protected] For more options, visit this group at http://groups.google.com/group/sage-devel URL: http://www.sagemath.org
