Opened 2 years ago

Last modified 14 months ago

#23880 new enhancement

Untangle customisation of element creation

Reported by: SimonKing Owned by:
Priority: major Milestone: sage-8.1
Component: coercion Keywords: element constructor
Cc: nthiery, mderickx, mantepse Merged in:
Authors: Reviewers:
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

Currently, we have in Parent.__init__:

    def __init__(...):
        ...
        if element_constructor is None:
            self._set_element_constructor()
        else:
            self._element_constructor = element_constructor
            self._element_init_pass_parent = guess_pass_parent(self, element_constructor)

and Parent._set_element_constructor:

    def _set_element_constructor(self):
        """
        This function is used in translating from the old to the new coercion model.
        """
        try: #if hasattr(self, '_element_constructor_'):
            _element_constructor_ = self._element_constructor_
        except (AttributeError, TypeError):
            # Remark: A TypeError can actually occur;
            # it is a possible reason for "hasattr" to return False
            return
        assert callable(_element_constructor_)
        self._element_constructor = _element_constructor_
        self._element_init_pass_parent = guess_pass_parent(self, self._element_constructor)

and in the parent methods of Sets() we have

        @lazy_attribute
        def _element_constructor_(self):
            if hasattr(self, "element_class"):
                return self._element_constructor_from_element_class
            else:
                return NotImplemented

That's quite contorted. __init__ calls set_element_constructor. If the category framework is used, then at that point we have initialised the category, which is a sub-category of Sets(). Therefore, by virtue of the category framework, self actually has _element_constructor_ , from Sets.ParentMethods.

Then, _set_element_constructor assigns self._element_constructor_ to self._element_constructor without trailing underscore. And the lazy attribute _element_constructor_ inherited from the categories tries whether self has an element_class. This, in turn, is a lazy attribute that (in its default implementation) depends on whether self has the attribute Element.

That's bizarre and should be simplified.

Currently, it seems that there are too many ways to customize element creation: Overriding Parent.__call__ (very bad), overriding _element_constructor, overriding _element_constructor_, overriding element_class, definition .Element. I suppose some of these ways just exist for historic reasons and should be removed.

See: #23881, #23917, #24348, #24363, #26879

Change History (35)

comment:1 Changed 2 years ago by SimonKing

By the way: It is simply not true that _set_element_constructor "is used in translating from the old to the new coercion model". It is called unless the argument element_constructor is passed to Parent.__init__, which I guess is very rarely the case.

comment:2 Changed 2 years ago by SimonKing

As a first step, one could define a copy of Sets.ParentMethods._element_constructor_ directly for Parent. Or actually remove it from the category framework: If an object belongs to the category of sets, it is supposed to be implemented as a sub-class of Parent, not just of CategoryObject.

comment:3 follow-up: Changed 2 years ago by jdemeyer

Just pointing out that defining class Element or element_class will not work in all cases since a few parents (homsets are a good example) allow multiple element classes.

comment:4 follow-up: Changed 2 years ago by jdemeyer

Regarding

self._element_init_pass_parent = guess_pass_parent(self, self._element_constructor)

I always found this a bad hack. One simplification would be to always require passing the parent.

comment:5 in reply to: ↑ 3 Changed 2 years ago by SimonKing

Replying to jdemeyer:

Just pointing out that defining class Element or element_class will not work in all cases since a few parents (homsets are a good example) allow multiple element classes.

Of course we have to discuss what customization we shall allow. The minimum we want to have is this:

  • If _element_constructor_ is defined by the user, then it will be used for element construction. If multiple element classes are involved (homsets), then that's what one has to do.
  • If .Element is assigned and the category framework is invoked, then element_class should be available as well and it should be a possible way to implement element creation.

But I am not so sure whether we should allow to override the default implementation of element_class. If we could agree that defining _element_constructor_ or .Element are the only customisation hooks, we would already have a substantial simplification.

Last edited 2 years ago by SimonKing (previous) (diff)

comment:6 in reply to: ↑ 4 Changed 2 years ago by SimonKing

Replying to jdemeyer:

Regarding

self._element_init_pass_parent = guess_pass_parent(self, self._element_constructor)

I always found this a bad hack. One simplification would be to always require passing the parent.

It depends. If you allow, as one way of customisation, to pass element_constructor as an argument of Parent.__init__ or to populate_coercion_lists (which, I believe, should not be the allowed!), then you need that check sooner or later.

comment:7 Changed 2 years ago by SimonKing

Example: Do we want this?

src/sage/modular/quatalg/brandt.py:        self._populate_coercion_lists_(coerce_list=[self.free_module()], element_constructor=BrandtModuleElement)

If it is possible to use the category framework then one should simply assign Element. Or one should define _element_constructor_ by returning an instance of BrandtModuleElement.

comment:8 follow-up: Changed 2 years ago by jdemeyer

I would argue that Parent._element_constructor is the way how a Parent internally stores how to construct elements. There is nothing wrong with an attribute like that, but it should be considered very private.

comment:9 Changed 2 years ago by jdemeyer

  • Dependencies set to #23881

comment:10 in reply to: ↑ 8 Changed 2 years ago by SimonKing

  • Dependencies #23881 deleted

Replying to jdemeyer:

I would argue that Parent._element_constructor is the way how a Parent internally stores how to construct elements. There is nothing wrong with an attribute like that, but it should be considered very private.

But what is Parent._element_constructor? It either is a callable that is passed during Parent.__init__ or Parent._populate_coercion_lists, or it is the same as _element_constructor_. If we disallowed passing an element constructor as an argument, there would be no difference between the version with and without trailing underscore.

Does the following scheme make sense to you?

  • _element_constructor (without trailing underscore) should be removed.
  • There should be a default implementation of _element_constructor_ for Parent (it currently does not exist). As a backup, we might keep a copy in Sets.ParentMethods. The default implementation passes the arguments to self.element_class and add parent to these arguments. It is one hook for customisation.
  • The comment "This probably should go into Sets().Parent" should be removed. All objects in the category of sets should be implemented as a subclass of Parent. The default implementation of element_class should stay as it is, thus, rely on Element, which is one hook for customisation (the category used in initialising the parent is another hook). The element class has to accept parent as a named argument.
  • Remove guess_pass_parent. self(*args,**kwds) should (after coercion) call self._element_constructor_, and this has the responsibility to pass self to whatever class is used for the elements.
Last edited 2 years ago by SimonKing (previous) (diff)

comment:11 Changed 2 years ago by SimonKing

Why do you think #23881 is needed?

comment:12 Changed 2 years ago by jdemeyer

I was thinking of #23881 as an intermediate step. You could even think of it as an "exercise" to understand this stuff better.

comment:13 Changed 2 years ago by jdemeyer

But if you don't like #23881, I won't insist (but then don't expect much help from me on #23880 either).

comment:14 Changed 2 years ago by mderickx

  • Cc mderickx added

comment:15 follow-up: Changed 2 years ago by tscrim

  • Description modified (diff)

Isn't one of the reasons we have this is because we still have some old-style parents not setting their category lingering around?

Also, all categories are (currently) subcategories of Sets (i.e., are concrete categories), and to be a parent, you have to be in the category of Sets. So I removed the "usually" in the description.

Last edited 2 years ago by tscrim (previous) (diff)

comment:16 follow-up: Changed 2 years ago by jdemeyer

What do categories have to do with this?

comment:17 in reply to: ↑ 15 Changed 2 years ago by SimonKing

Replying to tscrim:

Isn't one of the reasons we have this is because we still have some old-style parents not setting their category lingering around?

Also, all categories are (currently) subcategories of Sets (i.e., are concrete categories), and to be a parent, you have to be in the category of Sets.

sage: Objects().is_subcategory(Sets())
False

That's why I had it in the description.

comment:18 Changed 2 years ago by jdemeyer

  • Dependencies set to #23881

I am adding back the dependency on #23881. Even if it's not strictly a dependency, see it at least as a pointer: it contains some discussion and further dependencies.

comment:19 Changed 2 years ago by jdemeyer

  • Dependencies changed from #23881 to #23881, #23900

comment:20 in reply to: ↑ 16 Changed 2 years ago by SimonKing

  • Dependencies #23881, #23900 deleted

Replying to jdemeyer:

What do categories have to do with this?

See description. This ticket is about simplifying the customisation logic of element construction. It seems that the underlying reason for the current contortion is that the current implementation makes it possible to let the category framework be involved.

Let me elaborate on it.

  • One can imagine that it makes sense to provide a generic _element_constructor_ for some categories.
  • But methods of Parent would have precedence over WhateverCategory.ParentMethods.
  • Supposedly the above was the reason to not implement the most generic _element_constructor_ in Parent: A customisation via categories is only possible if Parent does not have a generic implementation.

So, we have first to answer this question: Do we want that an object gets its element constructor from its category? Currently, the category framework provides `_element_constructor_" in semigroups, sets and facade_sets. The one from sets belongs (I believe) to Parent. I didn't look at the other two yet. In any case, the current category framework makes little use of that possibility.

comment:21 Changed 2 years ago by SimonKing

  • Dependencies set to #23881, #23900

comment:22 follow-up: Changed 2 years ago by SimonKing

In the ticket description is not mentioned why I opened the ticket. I think it was because of a slowness in parent creation that a user observed. But I cannot find the relevant sage-devel or sage-support thread. Can you help me with that?

If there is actually no slowness in parent creation and all contortion just serves at providing a fast element creation (by having a cdef __create_element attribute) that is customisable on the level of categories, then perhaps #23881 and related tickets are enough and this ticket is invalid. What do you think?

comment:23 Changed 2 years ago by jdemeyer

  • Dependencies changed from #23881, #23900 to #23881, #23900, #23902

comment:24 follow-up: Changed 2 years ago by jdemeyer

Regardless of the reason why you opened the ticket, it's true that the current situation is a mess. I am generally +1 on cleaning things up, even if it serves no higher purpose.

comment:25 Changed 2 years ago by jdemeyer

  • Dependencies changed from #23881, #23900, #23902 to #23881, #23900, #23902, #23903

comment:26 in reply to: ↑ 24 ; follow-up: Changed 2 years ago by SimonKing

Replying to jdemeyer:

Regardless of the reason why you opened the ticket, it's true that the current situation is a mess. I am generally +1 on cleaning things up, even if it serves no higher purpose.

Agreed. However, if "cleaning up the mess" is only possible by making it impossible to customise element creation via categories, then I'd be against it - unless the clean up also implies a speed up.

Hence, without an example of a slow parent creation, I would think that #23881 and the other tickets do provide a simplification of the customisation logic and that it is not needed, e.g., to have a default Parent._element_constructor_.

comment:27 in reply to: ↑ 26 Changed 2 years ago by SimonKing

Replying to SimonKing:

Hence, without an example of a slow parent creation, I would think that #23881 and the other tickets do provide a simplification of the customisation logic...

Or perhaps not. For example, removing the element_constructor argument from Parent.__init__ and Parent._populate_coercion_lists is not covered by the other tickets and could be done here. Parent._element_constructor_ is a different story.

comment:28 in reply to: ↑ 22 Changed 2 years ago by mantepse

Replying to SimonKing:

In the ticket description is not mentioned why I opened the ticket. I think it was because of a slowness in parent creation that a user observed. But I cannot find the relevant sage-devel or sage-support thread. Can you help me with that?

That was me (Martin Rubey = mantepse), the slowness I observed is where the time is spentin Set([1,2,3]), originally in #23873 and, more precisely in https://trac.sagemath.org/ticket/23877#comment:9 and the comments above that.

comment:29 Changed 2 years ago by mantepse

Let me just repeat the (possibly) relevant profiles. If I read it correctly, almost all the time is spent in Parent.__init__.

sage: S = Subsets(range(100))
sage: l = [list(S.random_element()) for i in range(1000)]
sage: %prun [Set(b) for b in l]
         11005 function calls in 0.179 seconds

   Ordered by: internal time

   ncalls  tottime  percall  cumtime  percall filename:lineno(function)
     1000    0.101    0.000    0.146    0.000 set.py:255(__init__)
     1000    0.029    0.000    0.029    0.000 {hasattr}
     1000    0.015    0.000    0.171    0.000 set.py:88(Set)
     1000    0.010    0.000    0.011    0.000 dynamic_class.py:127(dynamic_class)
     1000    0.008    0.000    0.153    0.000 set.py:733(__init__)
        1    0.008    0.008    0.179    0.179 <string>:1(<module>)
     4000    0.006    0.000    0.006    0.000 {isinstance}
     1000    0.002    0.000    0.031    0.000 sets_cat.py:950(_element_constructor_)
     1000    0.000    0.000    0.000    0.000 {sage.rings.integer.is_Integer}
        3    0.000    0.000    0.000    0.000 vt100_input.py:275(_input_parser_generator)
        1    0.000    0.000    0.000    0.000 {method 'disable' of '_lsprof.Profiler' objects}
 sage: 
sage: %lprun -f sage.sets.set.Set_object.__init__ [Set(b) for b in l]
Timer unit: 1e-06 s

Total time: 0.158186 s
File: /home/martin/sage-develop/local/lib/python2.7/site-packages/sage/sets/set.py
Function: __init__ at line 255

Line #      Hits         Time  Per Hit   % Time  Line Contents
==============================================================
   255                                               def __init__(self, X, category=None):
   ...
   278      1000        18493     18.5     11.7          from sage.rings.integer import is_Integer
   279      1000         6255      6.3      4.0          if isinstance(X, integer_types) or is_Integer(X):
   280                                                       # The coercion model will try to call Set_object(0)
   281                                                       raise ValueError('underlying object cannot be an integer')
   282                                           
   283      1000         2248      2.2      1.4          if category is None:
   284                                                       category = Sets()
   285      1000       127575    127.6     80.6          Parent.__init__(self, category=category)
   286      1000         3615      3.6      2.3          self.__object = X

comment:30 Changed 2 years ago by jdemeyer

Various tickets need review: #23884, #23899, #23900, #23902, #23903.

These are all very simple tickets, each changing one very specific thing.

comment:31 Changed 2 years ago by mantepse

  • Cc mantepse added

comment:32 Changed 2 years ago by jdemeyer

  • Dependencies changed from #23881, #23900, #23902, #23903 to #23881, #23917

comment:33 Changed 2 years ago by jdemeyer

  • Description modified (diff)

comment:34 Changed 14 months ago by jdemeyer

  • Description modified (diff)

comment:35 Changed 14 months ago by jdemeyer

  • Dependencies #23881, #23917 deleted
Note: See TracTickets for help on using tickets.