Opened 4 years ago

Closed 4 years ago

#18361 closed enhancement (fixed)

CategoryObject: never cache gens_dict

Reported by: jdemeyer 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) Commit: 1c26046a6ae31a03ea375b9afa1e7f7ab444ecba
Dependencies: Stopgaps:

Description (last modified by 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".

Change History (22)

comment:1 Changed 4 years ago by jdemeyer

  • Description modified (diff)

comment:2 Changed 4 years ago by jdemeyer

  • Branch set to u/jdemeyer/categoryobject__always_cache_gens_dict

comment:3 Changed 4 years ago by jdemeyer

  • Commit set to 10fa09b08c316505bf42d99a45a524663e3c22fb
  • Status changed from new to needs_review

New commits:

10fa09bMove _gens_dict attribute to CategoryObject

comment:4 Changed 4 years ago by mmezzarobba

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

  • Commit changed from 10fa09b08c316505bf42d99a45a524663e3c22fb to 9154c9fd6f3d5df75e84e80cd2f1446909658915

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

9154c9fFix doctest

comment:6 Changed 4 years ago by jdemeyer

  • 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 follow-ups: Changed 4 years ago by vdelecroix

  • 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

Last edited 4 years ago by vdelecroix (previous) (diff)

comment:8 in reply to: ↑ 7 ; follow-ups: Changed 4 years ago by 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?

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 4 years ago by vdelecroix

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 4 years ago by jdemeyer

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 4 years ago by jdemeyer

  • 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. 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 4 years ago by git

  • Commit changed from 9154c9fd6f3d5df75e84e80cd2f1446909658915 to 1b5fff6a230bbb54967046ef4c7a23d27e12d4ce

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

1b5fff6Remove unused attribute CategoryObject._cdata

comment:13 Changed 4 years ago by jdemeyer

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 follow-up: Changed 4 years ago by jdemeyer

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

comment:15 in reply to: ↑ 14 ; follow-up: Changed 4 years ago by vdelecroix

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 4 years ago by vdelecroix

  • Reviewers set to Vincent Delecroix
  • Status changed from needs_review to needs_work

comment:17 in reply to: ↑ 15 Changed 4 years ago by jdemeyer

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

  • Commit changed from 1b5fff6a230bbb54967046ef4c7a23d27e12d4ce to 1c26046a6ae31a03ea375b9afa1e7f7ab444ecba

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

1c26046Remove caching of gens_dict()

comment:19 Changed 4 years ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:20 Changed 4 years ago by jdemeyer

  • Description modified (diff)
  • Summary changed from CategoryObject: always cache gens_dict to CategoryObject: never cache gens_dict

comment:21 Changed 4 years ago by vdelecroix

  • Milestone changed from sage-6.7 to sage-6.8
  • Status changed from needs_review to positive_review

comment:22 Changed 4 years ago by vbraun

  • Branch changed from u/jdemeyer/categoryobject__always_cache_gens_dict to 1c26046a6ae31a03ea375b9afa1e7f7ab444ecba
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.