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, aTypeError
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 tohash
, 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)
Change History (8)
comment:1 Changed 8 years ago by
- Status changed from new to needs_review
comment:2 Changed 8 years ago by
Changed 8 years ago by
comment:3 Changed 8 years ago by
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
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
- Type changed from PLEASE CHANGE to enhancement
comment:6 Changed 8 years ago by
- 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
- Merged in set to sage-5.11.beta0
- Resolution set to fixed
- Status changed from positive_review to closed
+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.