Opened 3 years ago

Closed 3 years ago

#22035 closed defect (fixed)

Fix do_pickle of cached_in_parent_method

Reported by: saraedum Owned by:
Priority: major Milestone: sage-8.0
Component: misc Keywords: days85
Cc: Merged in:
Authors: Julian Rüth Reviewers: Florent Hivert
Report Upstream: N/A Work issues:
Branch: 7ea68d7 (Commits) Commit: 7ea68d7991780ee40b3a19dc767673f7456d4a5b
Dependencies: Stopgaps:

Description

The do_pickle parameter was not properly passed on in CachedInParentMethod so that parameter actually never worked (the default was used instead which meant no pickling of caches.) The doctest did not catch it since the parent was not pickled and unpickled because _parent of the class had been assigned, not of the instance.

Change History (11)

comment:1 Changed 3 years ago by saraedum

  • Branch set to u/saraedum/introduce__cached_in_argument_method

comment:2 Changed 3 years ago by saraedum

  • Branch u/saraedum/introduce__cached_in_argument_method deleted
  • Status changed from new to needs_review

comment:3 Changed 3 years ago by saraedum

  • Branch set to u/saraedum/introduce__cached_in_argument_method
  • Commit set to 7162b738df71af32e8c98eecd1cc2a6412e8ac2b

New commits:

7162b73Forward do_pickle to CachedFunction's constructor

comment:4 follow-up: Changed 3 years ago by msaaltink

This seems like an obvious change and looks like the right thing to do.

There is a small anomaly: because the cache is in the parent, and the parent can be shared my many instances, the do_pickle parameter does not always apply as hoped. Admittedly this is in corner cases, like this one

SP_parent = MyParent()

class SP_pickle(object):
     def parent(self): return SP_parent
     @cached_in_parent_method(do_pickle=True)
     def f(self, x):
          return x

class SP_no_pickle(object):
     def parent(self): return SP_parent
     @cached_in_parent_method(do_pickle=False)
     def f(self, x):
          return x

Here, if instances of SP_pickle and of SP_no_pickle are created, whether the cache is pickled or not depends on which instance's f was called first. I'm not quite sure whether this is a problem, and if so, whether it is bad enough to bother fixing.

comment:5 in reply to: ↑ 4 Changed 3 years ago by hivert

  • Reviewers set to Florent Hivert,

Replying to msaaltink:

There is a small anomaly: because the cache is in the parent, and the parent can be shared my many instances, the do_pickle parameter does not always apply as hoped. Admittedly this is in corner cases, like this one

Here, if instances of SP_pickle and of SP_no_pickle are created, whether the cache is pickled or not depends on which instance's f was called first. I'm not quite sure whether this is a problem, and if so, whether it is bad enough to bother fixing.

In my opinion, the fact that the cache should be pickled or not is a choice that should be made made by the parent. Due to the way we write the cached_in_parent decorator, it appears syntactically to be a choice of the element, but I think it really belongs to the parent. As a consequence I won't bother taking care of this very corner case. Actually, I would consider it broken.

Anyway, IHMO the winning argument is that, we don't have that much use cases for cached_in_parent (all of them being related to root systems, by the way...) and even less use cases for parent with two different classes of elements.

So I'm giving a positive review after fixing the merge.

comment:6 Changed 3 years ago by hivert

  • Branch changed from u/saraedum/introduce__cached_in_argument_method to u/hivert/introduce__cached_in_argument_method

comment:7 Changed 3 years ago by hivert

  • Commit changed from 7162b738df71af32e8c98eecd1cc2a6412e8ac2b to 7ea68d7991780ee40b3a19dc767673f7456d4a5b
  • Status changed from needs_review to positive_review

New commits:

7ea68d7Merge branch 'develop' into t/22035/introduce__cached_in_argument_method

comment:8 Changed 3 years ago by tscrim

  • Keywords days85 added
  • Milestone changed from sage-7.5 to sage-8.0

comment:9 Changed 3 years ago by vbraun

  • Status changed from positive_review to needs_work

Reviewer name ends with a comma, is that intended? ;-)

comment:10 Changed 3 years ago by tscrim

  • Reviewers changed from Florent Hivert, to Florent Hivert
  • Status changed from needs_work to positive_review

comment:11 Changed 3 years ago by vbraun

  • Branch changed from u/hivert/introduce__cached_in_argument_method to 7ea68d7991780ee40b3a19dc767673f7456d4a5b
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.