CategoryObject: never cache gens_dict

[jdemeyer]

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".

Component: categories

Author: Jeroen Demeyer

Branch/Commit: 1c26046

Reviewer: Vincent Delecroix

Issue created by migration from https://trac.sagemath.org/ticket/18361

[jdemeyer]

Branch: u/jdemeyer/categoryobject__always_cache_gens_dict

[jdemeyer]

New commits:

10fa09b

Move _gens_dict attribute to CategoryObject

[jdemeyer]

Commit: 10fa09b

[mezzarobba]

comment:4

The patchbot complains about the usual example involving dir(Parent) in coercion_and_categories.rst.

[sagetrac-git]

Changed commit from 10fa09b to 9154c9f

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

9154c9f

Fix doctest

[jdemeyer]

comment:6

Thanks. The patchbot is (temporarily) down, so I cannot see if there are other failures.

[videlec]

comment:7

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

[jdemeyer]

comment:8

Replying to @videlec:

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.

[videlec]

comment:9

Replying to @jdemeyer:

Replying to @videlec:

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.

[jdemeyer]

comment:10

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/sage-config/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 496, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/usr/local/src/sage-config/local/lib/python2.7/site-packages/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'
**********************************************************************

[jdemeyer]

comment:11

Replying to @videlec:

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?

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.

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

1b5fff6

Remove unused attribute CategoryObject._cdata

[sagetrac-git]

Changed commit from 9154c9f to 1b5fff6

[jdemeyer]

comment:13

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.

[jdemeyer]

comment:14

Would you prefer to remove the caching completely in gens_dict()?

[videlec]

comment:15

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

[videlec]

Reviewer: Vincent Delecroix

[jdemeyer]

comment:17

Replying to @videlec:

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.

[sagetrac-git]

Changed commit from 1b5fff6 to 1c26046

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

1c26046

Remove caching of gens_dict()

[vbraun]

Changed branch from u/jdemeyer/categoryobject__always_cache_gens_dict to 1c26046