Opened 21 months ago

Last modified 3 months ago

#29037 new defect

cached_method does not work when the result of the method is None

Reported by: tscrim Owned by:
Priority: major Milestone: sage-9.5
Component: performance Keywords: cached method
Cc: jdemeyer, SimonKing, saraedum, klee Merged in:
Authors: Reviewers:
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description

sage: class Foo(object):
....:     def __init__(self):
....:         self.x = None
....:     @cached_method
....:     def bar(self):
....:         if self.x is None:
....:             return None
....:         return 5
....:     
sage: f = Foo()
sage: f.bar()
sage: f.x = -2
sage: f.bar()
5
sage: class Foo2(object):
....:     def __init__(self):
....:         self.x = None
....:     @cached_method
....:     def bar(self):
....:         if self.x is None:
....:             return -1
....:         return 5
....:     
sage: f2 = Foo2()
sage: f2.bar()
-1
sage: f2.x = 0
sage: f2.bar()
-1

Sometimes it is useful to do a long computation and return None (say, because you realize something is impossible) and then store that computation. Furthermore, this is not a documented special case AFAIK, so even if we do not change the behavior, we need to add it to the documentation.

Change History (7)

comment:1 in reply to: ↑ description Changed 21 months ago by SimonKing

Replying to tscrim:

Sometimes it is useful to do a long computation and return None (say, because you realize something is impossible) and then store that computation. Furthermore, this is not a documented special case AFAIK...

Yes it is documented:

cdef class CachedMethodCallerNoArgs(CachedFunction):
    """
    Utility class that is used by :class:`CachedMethod` to bind a
    cached method to an instance, in the case of a method that does
    not accept any arguments except ``self``.

    .. NOTE::

        The return value ``None`` would not be cached. So, if you have
        a method that does not accept arguments and may return ``None``
        after a lengthy computation, then ``@cached_method`` should not
        be used.
...

comment:2 Changed 21 months ago by tscrim

I see; I didn't think of looking there. Perhaps we can make that more prominent, perhaps in the cached_method decorator?

comment:3 Changed 18 months ago by mkoeppe

  • Milestone changed from sage-9.1 to sage-9.2

Batch modifying tickets that will likely not be ready for 9.1, based on a review of the ticket title, branch/review status, and last modification date.

comment:4 Changed 14 months ago by mkoeppe

  • Cc klee added

comment:5 Changed 14 months ago by mkoeppe

  • Milestone changed from sage-9.2 to sage-9.3

comment:6 Changed 8 months ago by mkoeppe

  • Milestone changed from sage-9.3 to sage-9.4

Setting new milestone based on a cursory review of ticket status, priority, and last modification date.

comment:7 Changed 3 months ago by mkoeppe

  • Milestone changed from sage-9.4 to sage-9.5
Note: See TracTickets for help on using tickets.