Opened 8 years ago

Closed 8 years ago

#14524 closed enhancement (fixed)

Decorator for methods requiring mutability or immutability

Reported by: SimonKing Owned by: jason
Priority: major Milestone: sage-5.11
Component: misc Keywords: decorator mutability
Cc: Merged in: sage-5.11.beta0
Authors: Simon King Reviewers: Volker Braun
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

Some methods (most notably __hash__) require that an object is immutable. Other methods (such as add_vertex of a graph) require that an object is mutable.

I suggest to introduce a method decorator, such that (im)mutability is automatically verified before calling the method. Note that the patch just creates the decorator, but does not use it yet. In a second step, I plan to use it on graphs.

The decorator tests immutability by the value of an attribute called _is_immutable, which is used by sage.structure.mutability.Mutability anyway (but I change this attribute into a public attribute). Unfortunately, sage.graphs.generic_graph.GenericGraph.__hash__ uses another name, namely _immutable. But I think this can be changed in the second step.

From the doctests:

        sage: from sage.structure.mutability import require_mutable, require_immutable
        sage: class A:
        ...    def __init__(self, val):
        ...        self._m = val
        ...    @require_mutable
        ...    def change(self, new_val):
        ...        'change self'
        ...        self._m = new_val
        ...    @require_immutable
        ...    def __hash__(self):
        ...        'implement hash'
        ...        return hash(self._m)
        sage: a = A(5)
        sage: a.change(6)
        sage: hash(a)
        Traceback (most recent call last):
        ...
        AssertionError: <type 'instance'> instance is mutable, <function __hash__ at ...> must not be called
        sage: a._is_immutable = True
        sage: hash(a)
        6
        sage: a.change(7)   # indirect doctest
        Traceback (most recent call last):
        ...
        AssertionError: <type 'instance'> instance is immutable, <function change at ...> must not be called
        sage: from sage.misc.sageinspect import sage_getdoc
        sage: print sage_getdoc(a.change)
        change self

As one can see, by default an object is supposed to be mutable. This may be important during initialisation of the object. So, one would first initialise it and then declare that it is (from now on) immutable.

Questions

  • Is AssertionError the right thing to raise here? Usually, when hashing does not work, a TypeError is raised.
  • One could be more intrusive. For example, when calling a method decorated by require_immutable on a mutable object, one could instead make it immutable (by assignment to _is_immutable. In that way, after the first call to hash, the object can not be accidentally changed (but of course, if the user wants so, _is_immutable can be deleted). Good idea, or not pythonic?

Attachments (1)

trac14524-immutability_decorators.patch (5.0 KB) - added by SimonKing 8 years ago.

Download all attachments as: .zip

Change History (8)

comment:1 Changed 8 years ago by SimonKing

  • Status changed from new to needs_review

comment:2 Changed 8 years ago by vbraun

+1 for TypeError, or perhaps subclass MutabilityError(TypeError) so you can just try/except on the mutability.

If you also want to change the mutability in the decorator then you should provide additional ones like @force_immutable. Though this might be overengineering things.

Changed 8 years ago by SimonKing

comment:3 Changed 8 years ago by SimonKing

I have changed it into ValueError, because this is what is raised in Mutability._require_mutable as well.

I agree that it would be over-engineering to change mutability when hash is called.

comment:4 Changed 8 years ago by SimonKing

By the way, here is some data on the overhead:

sage: from sage.structure.mutability import require_mutable
sage: class A:
....:     def bla(self):
....:         return 1
....:     @require_mutable
....:     def blabla(self):
....:         return 1
....:     
sage: a = A()
sage: %timeit x=a.bla()
1000000 loops, best of 3: 404 ns per loop
sage: %timeit x=a.blabla()
1000000 loops, best of 3: 1.78 us per loop
sage: a._is_immutable = False
sage: %timeit x=a.blabla()
1000000 loops, best of 3: 711 ns per loop

comment:5 Changed 8 years ago by SimonKing

  • Type changed from PLEASE CHANGE to enhancement

comment:6 Changed 8 years ago by vbraun

  • Milestone changed from sage-5.10 to sage-5.11
  • Reviewers set to Volker Braun
  • Status changed from needs_review to positive_review

Looks good to me.

comment:7 Changed 8 years ago by jdemeyer

  • Merged in set to sage-5.11.beta0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.