Opened 8 years ago

Closed 8 years ago

#18361 closed enhancement (fixed)

CategoryObject: never cache gens_dict

Reported by: Jeroen Demeyer Owned by:
Priority: major Milestone: sage-6.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:

Status badges

Description (last modified by Jeroen Demeyer)

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 8 years ago by Jeroen Demeyer

Description: modified (diff)

comment:2 Changed 8 years ago by Jeroen Demeyer

Branch: u/jdemeyer/categoryobject__always_cache_gens_dict

comment:3 Changed 8 years ago by Jeroen Demeyer

Commit: 10fa09b08c316505bf42d99a45a524663e3c22fb
Status: newneeds_review

New commits:

10fa09bMove _gens_dict attribute to CategoryObject

comment:4 Changed 8 years ago by Marc Mezzarobba

Status: needs_reviewneeds_work

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

comment:5 Changed 8 years ago by git

Commit: 10fa09b08c316505bf42d99a45a524663e3c22fb9154c9fd6f3d5df75e84e80cd2f1446909658915

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

9154c9fFix doctest

comment:6 Changed 8 years ago by Jeroen Demeyer

Status: needs_workneeds_review

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

comment:7 Changed 8 years ago by Vincent Delecroix

Status: needs_reviewneeds_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

Last edited 8 years ago by Vincent Delecroix (previous) (diff)

comment:8 in reply to:  7 ; Changed 8 years ago by Jeroen Demeyer

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 8 years ago by Vincent Delecroix

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 8 years ago by Jeroen Demeyer

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'
**********************************************************************

comment:11 in reply to:  7 Changed 8 years ago by Jeroen Demeyer

Status: needs_infoneeds_review

Replying to vdelecroix:

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.

comment:12 Changed 8 years ago by git

Commit: 9154c9fd6f3d5df75e84e80cd2f14469096589151b5fff6a230bbb54967046ef4c7a23d27e12d4ce

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

1b5fff6Remove unused attribute CategoryObject._cdata

comment:13 Changed 8 years ago by Jeroen Demeyer

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 Changed 8 years ago by Jeroen Demeyer

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

comment:15 in reply to:  14 ; Changed 8 years ago by Vincent Delecroix

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 8 years ago by Vincent Delecroix

Reviewers: Vincent Delecroix
Status: needs_reviewneeds_work

comment:17 in reply to:  15 Changed 8 years ago by Jeroen Demeyer

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 8 years ago by git

Commit: 1b5fff6a230bbb54967046ef4c7a23d27e12d4ce1c26046a6ae31a03ea375b9afa1e7f7ab444ecba

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

1c26046Remove caching of gens_dict()

comment:19 Changed 8 years ago by Jeroen Demeyer

Status: needs_workneeds_review

comment:20 Changed 8 years ago by Jeroen Demeyer

Description: modified (diff)
Summary: CategoryObject: always cache gens_dictCategoryObject: never cache gens_dict

comment:21 Changed 8 years ago by Vincent Delecroix

Milestone: sage-6.7sage-6.8
Status: needs_reviewpositive_review

comment:22 Changed 8 years ago by Volker Braun

Branch: u/jdemeyer/categoryobject__always_cache_gens_dict1c26046a6ae31a03ea375b9afa1e7f7ab444ecba
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.