Opened 7 years ago
Closed 7 years ago
#18361 closed enhancement (fixed)
CategoryObject: never cache gens_dict
Reported by:  jdemeyer  Owned by:  

Priority:  major  Milestone:  sage6.8 
Component:  categories  Keywords:  
Cc:  Merged in:  
Authors:  Jeroen Demeyer  Reviewers:  Vincent Delecroix 
Report Upstream:  N/A  Work issues:  
Branch:  1c26046 (Commits, GitHub, GitLab)  Commit:  1c26046a6ae31a03ea375b9afa1e7f7ab444ecba 
Dependencies:  Stopgaps: 
Description (last modified by )
The cdef class CategoryObject
has this strange method:
def gens_dict(self): r""" Return a dictionary whose entries are ``{var_name:variable,...}``. """ if HAS_DICTIONARY(self): try: if self._gens_dict is not None: return self._gens_dict except AttributeError: pass v = {} for x in self.gens(): v[str(x)] = x if HAS_DICTIONARY(self): self._gens_dict = v return v
which provides caching only for Python subclasses. It turns out that caching this doesn't really matter since gens_dict()
is not used in critical code, so we can just remove the funny "caching".
Change History (22)
comment:1 Changed 7 years ago by
 Description modified (diff)
comment:2 Changed 7 years ago by
 Branch set to u/jdemeyer/categoryobject__always_cache_gens_dict
comment:3 Changed 7 years ago by
 Commit set to 10fa09b08c316505bf42d99a45a524663e3c22fb
 Status changed from new to needs_review
comment:4 Changed 7 years ago by
 Status changed from needs_review to needs_work
The patchbot complains about the usual example involving dir(Parent)
in coercion_and_categories.rst
.
comment:5 Changed 7 years ago by
 Commit changed from 10fa09b08c316505bf42d99a45a524663e3c22fb to 9154c9fd6f3d5df75e84e80cd2f1446909658915
Branch pushed to git repo; I updated commit sha1. New commits:
9154c9f  Fix doctest

comment:6 Changed 7 years ago by
 Status changed from needs_work to needs_review
Thanks. The patchbot is (temporarily) down, so I cannot see if there are other failures.
comment:7 followups: ↓ 8 ↓ 11 Changed 7 years ago by
 Status changed from needs_review to needs_info
The class ParentWithGens
is deprecated! There is no need for an attribute _gens_dict
on all category objects. Why Partitions
or Integers
should have a method gens
or _gens_dict
attribute? I am very against this change.
Vincent
comment:8 in reply to: ↑ 7 ; followups: ↓ 9 ↓ 10 Changed 7 years ago by
Replying to vdelecroix:
The class
ParentWithGens
is deprecated! There is no need for an attribute_gens_dict
on all category objects.
Then why is there a gens_dict()
method on all category objects?
It makes absolutely no sense to have a method on class A
whose results are cached in a subclass B
.
If you prefer, I can move the whole gens_dict()
method, but that's a more drastic change.
comment:9 in reply to: ↑ 8 Changed 7 years ago by
Replying to jdemeyer:
Replying to vdelecroix:
The class
ParentWithGens
is deprecated! There is no need for an attribute_gens_dict
on all category objects.Then why is there a
gens_dict()
method on all category objects?
This is indeed the problem!
If you prefer, I can move the whole
gens_dict()
method, but that's a more drastic change.
Yes please. I found that doing what you proposed is just worse in the direction of cleaning Element/CategoryObject/Parent
.
comment:10 in reply to: ↑ 8 Changed 7 years ago by
Replying to jdemeyer:
If you prefer, I can move the whole
gens_dict()
method, but that's a more drastic change.
That doesn't work:
sage t src/sage/rings/polynomial/pbori.pyx ********************************************************************** File "src/sage/rings/polynomial/pbori.pyx", line 4067, in sage.rings.polynomial.pbori.BooleanPolynomial.subs Failed example: f.subs(x=1) Exception raised: Traceback (most recent call last): File "/usr/local/src/sageconfig/local/lib/python2.7/sitepackages/sage/doctest/forker.py", line 496, in _run self.compile_and_execute(example, compiler, test.globs) File "/usr/local/src/sageconfig/local/lib/python2.7/sitepackages/sage/doctest/forker.py", line 858, in compile_and_execute exec(compiled, globs) File "<doctest sage.rings.polynomial.pbori.BooleanPolynomial.subs[2]>", line 1, in <module> f.subs(x=Integer(1)) File "sage/rings/polynomial/pbori.pyx", line 4115, in sage.rings.polynomial.pbori.BooleanPolynomial.subs (build/cythonized/sage/rings/polynomial/pbori.cpp:32460) gdict = P._monom_monoid.gens_dict() File "sage/structure/parent.pyx", line 840, in sage.structure.parent.Parent.__getattr__ (build/cythonized/sage/structure/parent.c:7903) attr = getattr_from_other_class(self, self._category.parent_class, name) File "sage/structure/misc.pyx", line 253, in sage.structure.misc.getattr_from_other_class (build/cythonized/sage/structure/misc.c:1583) raise dummy_attribute_error AttributeError: 'BooleanMonomialMonoid_with_category' object has no attribute 'gens_dict' **********************************************************************
comment:11 in reply to: ↑ 7 Changed 7 years ago by
 Status changed from needs_info to needs_review
Replying to vdelecroix:
The class
ParentWithGens
is deprecated! There is no need for an attribute_gens_dict
on all category objects. WhyPartitions
orIntegers
should have a methodgens
or_gens_dict
attribute?
On the other hand, does it really hurt to have this extra attribute? We already have these attributes on CategoryObject
:
cdef class CategoryObject(SageObject): cdef _generators cdef _category cdef public _base cdef public _cdata cdef public _names # will be _printer cdef public _factory_data cdef object __weakref__ cdef long _hash_value
The point is: CategoryObject
already has functions dealing with generators, such as the _populate_generators_()
method. So why not gens_dict()
then?
And my proposed patch is actually compatible with the deprecation of ParentWithGens
, since it removes something from ParentWithGens
.
comment:12 Changed 7 years ago by
 Commit changed from 9154c9fd6f3d5df75e84e80cd2f1446909658915 to 1b5fff6a230bbb54967046ef4c7a23d27e12d4ce
Branch pushed to git repo; I updated commit sha1. New commits:
1b5fff6  Remove unused attribute CategoryObject._cdata

comment:13 Changed 7 years ago by
Let me remind that this ticket is not about the proper place to put the gens_dict()
method, but just about doing the caching properly.
comment:14 followup: ↓ 15 Changed 7 years ago by
Would you prefer to remove the caching completely in gens_dict()
?
comment:15 in reply to: ↑ 14 ; followup: ↓ 17 Changed 7 years ago by
Replying to jdemeyer:
Would you prefer to remove the caching completely in
gens_dict()
?
+1 to me. Building a dictionary is very fast. I had a quick look through the source code. And no functions relies on it for critical computations (mostly it is to build a polynomial from a string). Note that the fastest way I found to allocate a dictionary is through dict.fromkeys
.
Could you also remove the custom implementation in PolynomialRing
as the following seems to work
sage: Parent.gens_dict(QQ['x']) {'x': x} sage: Parent.gens_dict(QQ['x,y']) {'x': x, 'y': y}
Vincent
comment:16 Changed 7 years ago by
 Reviewers set to Vincent Delecroix
 Status changed from needs_review to needs_work
comment:17 in reply to: ↑ 15 Changed 7 years ago by
Replying to vdelecroix:
Could you also remove the custom implementation in
PolynomialRing
No, I disagree. The reason is that the PolynomialRing
implementation is much more elegant than the generic CategoryObject
implementation. If I could (but I cannot), I would keep only the PolynomialRing
implementation.
comment:18 Changed 7 years ago by
 Commit changed from 1b5fff6a230bbb54967046ef4c7a23d27e12d4ce to 1c26046a6ae31a03ea375b9afa1e7f7ab444ecba
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
1c26046  Remove caching of gens_dict()

comment:19 Changed 7 years ago by
 Status changed from needs_work to needs_review
comment:20 Changed 7 years ago by
 Description modified (diff)
 Summary changed from CategoryObject: always cache gens_dict to CategoryObject: never cache gens_dict
comment:21 Changed 7 years ago by
 Milestone changed from sage6.7 to sage6.8
 Status changed from needs_review to positive_review
comment:22 Changed 7 years ago by
 Branch changed from u/jdemeyer/categoryobject__always_cache_gens_dict to 1c26046a6ae31a03ea375b9afa1e7f7ab444ecba
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
Move _gens_dict attribute to CategoryObject