Remove Python access from Parent._element_constructor

[jdemeyer]

In order to avoid confusion between _element_constructor_ (a public interface) and _element_constructor (a private attribute), we change the latter from cdef public to cdef. The same for _element_init_pass_parent. This way, those attributes can no longer be accessed from Python.

Depends on #23882
Depends on #23884
Depends on #23894
Depends on #23899
Depends on #23905
Depends on #23907
Depends on #23914
Depends on #24033

CC: @simon-king-jena @koffie

Component: coercion

Author: Jeroen Demeyer, Simon King

Branch: 41614a7

Reviewer: Travis Scrimshaw

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

[simon-king-jena]

Replying to @jdemeyer:

In order to avoid confusion between _element_constructor_ (a public interface) and _element_constructor (a private attribute), we rename the latter.

I am not sure whether _element_constructor is needed at all. Since _element_constructor_ has underscores, it is private to a certain extent, but still public enough to be a hook for customisation. That's why I believe a default implementation (which is allowed to override) of _element_constructor_ should be a replacement of the current _element_constructor.

[jdemeyer]

comment:2

Replying to @simon-king-jena:

I am not sure whether _element_constructor is needed at all.

I never claimed that it is. Please see my last comments on #23880.

[simon-king-jena]

comment:3

Replying to @jdemeyer:

Replying to @simon-king-jena:

I am not sure whether _element_constructor is needed at all.

I never claimed that it is. Please see my last comments on #23880.

So, your suggestion is to rename _element_constructor into __construct_element in order to learn how things work? This could indeed have some benefits: Making it a very private argument, we would see errors if something tries to call it outside of sage.structure.parents (due to name mangling). This would indicate how much work it would be to avoid the indirection and directly replace the internal element constructor by the hook.

[jdemeyer]

comment:4

Replying to @simon-king-jena:

Making it a very private argument, we would see errors if something tries to call it outside of sage.structure.parents (due to name mangling).

Note that a Cython cdef classes doesn't do mangling, only Python classes do. So the split would be on the Cython/Python border.

[simon-king-jena]

comment:5

I guess the following is what we have to change:

$ grep "_element_constructor " -r src/sage
src/sage/combinat/subword.py:        if self._element_constructor is tuple:
src/sage/combinat/sf/new_kschur.py:    _element_constructor = _element_constructor_
src/sage/combinat/sf/new_kschur.py:    _element_constructor = _element_constructor_
src/sage/combinat/sf/new_kschur.py:    _element_constructor = _element_constructor_
src/sage/combinat/sf/new_kschur.py:    _element_constructor = _element_constructor_
src/sage/combinat/sf/k_dual.py:    _element_constructor = _element_constructor_
src/sage/categories/homset.py:        if self._element_constructor is None:
src/sage/categories/sets_cat.py:        # Todo: simplify the _element_constructor definition logic
src/sage/structure/coerce_maps.pyx:        if (<Parent>codomain)._element_constructor is None:
src/sage/structure/parent_base.pyx:    if p._element_constructor is not None:
src/sage/structure/parent.pyx:            self._element_constructor = element_constructor
src/sage/structure/parent.pyx:            sage: k = GF(5); k._element_constructor # indirect doctest
src/sage/structure/parent.pyx:        self._element_constructor = _element_constructor_
src/sage/structure/parent.pyx:        if self._element_constructor is None:
src/sage/structure/parent.pyx:                self._element_constructor = self._element_constructor_
src/sage/structure/parent.pyx:        self._element_constructor = element_constructor
src/sage/structure/parent_gens.pyx:    if p._element_constructor is not None:
src/sage/structure/parent_gens.pyx:        if self._element_constructor is not None:
src/sage/structure/parent_gens.pyx:        if self._element_constructor is not None:
src/sage/structure/parent_gens.pyx:        if self._element_constructor is not None:
src/sage/structure/parent_old.pyx:    if p._element_constructor is not None:
src/sage/structure/parent_old.pyx:        if self._element_constructor is not None:
src/sage/structure/parent_old.pyx:        if self._element_constructor is not None:
src/sage/structure/parent_old.pyx:        if self._element_constructor is None:
src/sage/structure/parent_old.pyx:        if self._element_constructor is None:
src/sage/structure/parent_old.pyx:        if self._element_constructor is None:
src/sage/structure/parent_old.pyx:        if self._element_constructor is None:
src/sage/structure/parent_old.pyx:                self._element_constructor = self._element_constructor_

and

$ grep "_element_constructor(" -r src/sage
src/sage/sets/cartesian_product.py:            ``self._element_constructor(elements)``.
src/sage/rings/padics/CA_template.pxi:        self._zero = R._element_constructor(R, 0)
src/sage/rings/padics/CA_template.pxi:        self._zero = R._element_constructor(R, 0)
src/sage/rings/padics/FM_template.pxi:        self._zero = R._element_constructor(R, 0)
src/sage/rings/padics/FM_template.pxi:        self._zero = R._element_constructor(R, 0)
src/sage/rings/padics/FP_template.pxi:        self._zero = <FPElement?>R._element_constructor(R, 0)
src/sage/rings/padics/FP_template.pxi:        self._zero = R._element_constructor(R, 0)
src/sage/rings/padics/FP_template.pxi:        self._zero = R._element_constructor(R, 0)
src/sage/rings/padics/CR_template.pxi:        self._zero = <CRElement?>R._element_constructor(R, 0)
src/sage/rings/padics/CR_template.pxi:        self._zero = R._element_constructor(R, 0)
src/sage/rings/padics/CR_template.pxi:        self._zero = R._element_constructor(R, 0)
src/sage/rings/finite_rings/hom_prime_finite_field.pyx:            return self._codomain._element_constructor(x)
src/sage/rings/finite_rings/hom_prime_finite_field.pyx:        return self._codomain._element_constructor(x)
src/sage/rings/polynomial/polynomial_element.pyx:            return self.__class__(P,[a], check=False) #P._element_constructor(a, check=False)
src/sage/combinat/integer_lists/lists.py:            sage: L._element_constructor([1,2,3])
src/sage/geometry/linear_expression.py:            sage: L._element_constructor([1, 2, 3], 4)
src/sage/geometry/linear_expression.py:            sage: L._element_constructor(vector(ZZ, [1, 2, 3]), 4)
src/sage/geometry/linear_expression.py:            sage: L._element_constructor(4)
src/sage/geometry/linear_expression.py:            sage: L._element_constructor(vector([4, 1, 2, 3]))
src/sage/geometry/linear_expression.py:            sage: L._element_constructor(m)
src/sage/geometry/hyperplane_arrangement/arrangement.py:            sage: L._element_constructor(polytopes.hypercube(2))
src/sage/categories/morphism.pyx:            return C._element_constructor(C, x, *args, **kwds)
src/sage/categories/morphism.pyx:            return C._element_constructor(x, *args, **kwds)
src/sage/modular/dirichlet.py:        self._set_element_constructor()
src/sage/structure/coerce_maps.pyx:            return C._element_constructor(C, x)
src/sage/structure/coerce_maps.pyx:                    return C._element_constructor(C, x)
src/sage/structure/coerce_maps.pyx:                    return C._element_constructor(C, x, **kwds)
src/sage/structure/coerce_maps.pyx:                    return C._element_constructor(C, x, *args)
src/sage/structure/coerce_maps.pyx:                    return C._element_constructor(C, x, *args, **kwds)
src/sage/structure/coerce_maps.pyx:            return C._element_constructor(x)
src/sage/structure/coerce_maps.pyx:                    return C._element_constructor(x)
src/sage/structure/coerce_maps.pyx:                    return C._element_constructor(x, **kwds)
src/sage/structure/coerce_maps.pyx:                    return C._element_constructor(x, *args)
src/sage/structure/coerce_maps.pyx:                    return C._element_constructor(x, *args, **kwds)
src/sage/structure/coerce_maps.pyx:        return C._element_constructor(self._call_(x), *args, **kwds)
src/sage/structure/parent.pyx:            self._set_element_constructor()
src/sage/structure/parent.pyx:    def _set_element_constructor(self):
src/sage/structure/parent_old.pyx:        self._set_element_constructor()

[simon-king-jena]

comment:6

Replying to @simon-king-jena:

$ grep "_element_constructor " -r src/sage
...
src/sage/combinat/sf/new_kschur.py:    _element_constructor = _element_constructor_
src/sage/combinat/sf/new_kschur.py:    _element_constructor = _element_constructor_
src/sage/combinat/sf/new_kschur.py:    _element_constructor = _element_constructor_
src/sage/combinat/sf/new_kschur.py:    _element_constructor = _element_constructor_
src/sage/combinat/sf/k_dual.py:    _element_constructor = _element_constructor_

The above is strange. Shouldn't Parent._set_element_constructor assign _element_constructor_ to _element_constructor anyway? So, why do it on class level?

$ grep "_element_constructor(" -r src/sage
...
src/sage/rings/padics/CA_template.pxi:        self._zero = R._element_constructor(R, 0)
src/sage/rings/padics/CA_template.pxi:        self._zero = R._element_constructor(R, 0)
src/sage/rings/padics/FM_template.pxi:        self._zero = R._element_constructor(R, 0)
src/sage/rings/padics/FM_template.pxi:        self._zero = R._element_constructor(R, 0)
src/sage/rings/padics/FP_template.pxi:        self._zero = <FPElement?>R._element_constructor(R, 0)
src/sage/rings/padics/FP_template.pxi:        self._zero = R._element_constructor(R, 0)
src/sage/rings/padics/FP_template.pxi:        self._zero = R._element_constructor(R, 0)
src/sage/rings/padics/CR_template.pxi:        self._zero = <CRElement?>R._element_constructor(R, 0)
src/sage/rings/padics/CR_template.pxi:        self._zero = R._element_constructor(R, 0)
src/sage/rings/padics/CR_template.pxi:        self._zero = R._element_constructor(R, 0)

Here we could probably use the default implementation of zero().

src/sage/combinat/integer_lists/lists.py:            sage: L._element_constructor([1,2,3])
src/sage/geometry/linear_expression.py:            sage: L._element_constructor([1, 2, 3], 4)
src/sage/geometry/linear_expression.py:            sage: L._element_constructor(vector(ZZ, [1, 2, 3]), 4)
src/sage/geometry/linear_expression.py:            sage: L._element_constructor(4)
src/sage/geometry/linear_expression.py:            sage: L._element_constructor(vector([4, 1, 2, 3]))
src/sage/geometry/linear_expression.py:            sage: L._element_constructor(m)
src/sage/geometry/hyperplane_arrangement/arrangement.py:            sage: L._element_constructor(polytopes.hypercube(2))

An internal attribute such as _element_constructor shouldn't be mentioned in the docs.

[simon-king-jena]

comment:7

Replying to @simon-king-jena:

src/sage/combinat/subword.py:        if self._element_constructor is tuple:

Why should it be a tuple?? Very strange.

[simon-king-jena]

comment:8

Replying to @simon-king-jena:

Replying to @simon-king-jena:

src/sage/combinat/subword.py:        if self._element_constructor is tuple:

Why should it be a tuple?? Very strange.

I see: It is not a tuple, but it is the class .

That's probably a tricky case: The elements are tuples, so, the elements are not instances of sage.structure.element.Element, and in particular it is one of the cases where we do not want to pass the parent. It should be possible to customize _element_constructor_ in that case, but there would be a slow-down, compared with calling the tuple constructor directly (without the additional step of a method that calls the tuple constructor).

[mantepse]

comment:9

Replying to @simon-king-jena:

Replying to @simon-king-jena:

src/sage/combinat/subword.py:        if self._element_constructor is tuple:

Why should it be a tuple?? Very strange.

looking at the last few lines of sagemath/sagetrac-mirror@a291564
maybe that should have been self._build

[simon-king-jena]

comment:10

Replying to @mantepse:

looking at the last few lines of sagemath/sagetrac-mirror@a291564
maybe that should have been self._build

Probably you are right. And the fact that it doesn't fail indicates that it isn't sufficiently tested.

[mantepse]

comment:11

Replying to @simon-king-jena:

Probably you are right. And the fact that it doesn't fail indicates that it isn't sufficiently tested.

I just checked, it's just a speedup of a factor 4 - I guess there is no performance testing in sage:

sage: S = Subwords([randint(1,10) for i in range(100)], 3, element_constructor=tuple)
sage: %timeit len([x for x in S.__iter__()])
1 loop, best of 3: 322 ms per loop
sage: %timeit len([x for x in S.__iter_new__()])
10 loops, best of 3: 85.7 ms per loop

sage: A = [x for x in S.__iter_new__()]
sage: B = [x for x in S.__iter__()]
sage: A == B
True

[simon-king-jena]

comment:12

Replying to @mantepse:

sage: A = [x for x in S.iter_new()]

What is __iter_new__?

[mantepse]

comment:13

Replying to @simon-king-jena:

Replying to @mantepse:

sage: A = [x for x in S.iter_new()]

What is __iter_new__?

That's the new (correct) implementation - I always put in old and new in parallel for the comparison. That's #23882 now.

[simon-king-jena]

Dependencies: #23882

[simon-king-jena]

comment:15

On top of #23882, I propose to first remove _element_constructor from the docs, before we rename it in the code.

[mantepse]

comment:16

I think the following grep is also needed:

grep -r --color -nH "_element_constructor$"  *
categories/sets_cat.py:964:                sage: A._element_constructor
categories/sets_cat.py:970:                sage: B._element_constructor
combinat/integer_lists/lists.py:173:        a = self._element_constructor
combinat/integer_lists/lists.py:174:        b = other._element_constructor
combinat/integer_lists/lists.py:281:            sage: L._element_constructor
structure/parent.pxd:13:    cdef public _element_constructor
structure/parent.pyx:776:        d['_element_constructor'] = self._element_constructor
structure/parent.pyx:1869:                element_constructor = (<Parent>S)._element_constructor

[mantepse]

comment:17

and another one:

grep -r --color -nH "_element_constructor ="  *
combinat/sf/k_dual.py:923:    _element_constructor = _element_constructor_
combinat/sf/new_kschur.py:1021:    _element_constructor = _element_constructor_
combinat/sf/new_kschur.py:1279:    _element_constructor = _element_constructor_
combinat/sf/new_kschur.py:1395:    _element_constructor = _element_constructor_
combinat/sf/new_kschur.py:1472:    _element_constructor = _element_constructor_
structure/parent_old.pyx:397:                self._element_constructor = self._element_constructor_
structure/parent.pyx:316:            self._element_constructor = element_constructor
structure/parent.pyx:607:        self._element_constructor = _element_constructor_
structure/parent.pyx:914:                self._element_constructor = self._element_constructor_
structure/parent.pyx:1452:        self._element_constructor = element_constructor

[simon-king-jena]

comment:18

The documentation already is a lot worse than I thought:

$ grep "_element_constructor" -r src/sage | grep "sage\:"
src/sage/schemes/toric/variety.py:            sage: H._element_constructor_(1)
src/sage/schemes/toric/divisor.py:            sage: TDiv._element_constructor_([ (1,P2.gen(2)) ])
src/sage/schemes/toric/divisor.py:            sage: Cl._element_constructor_(D)
src/sage/schemes/toric/homset.py:            sage: hom_set(fm)     # calls hom_set._element_constructor_()
src/sage/schemes/product_projective/homset.py:            sage: P(QQ)._element_constructor_([4, 2, 2, 0])
src/sage/schemes/generic/homset.py:            sage: H._element_constructor_(f)
src/sage/schemes/generic/homset.py:            sage: F_points._element_constructor_([4,2*a])
src/sage/schemes/projective/projective_homset.py:            sage: X._element_constructor_([0,1,0])
src/sage/schemes/affine/affine_homset.py:            sage: H._element_constructor_(ring_hom)
src/sage/numerical/linear_tensor.py:            sage: LT._element_constructor_(123)
src/sage/numerical/linear_functions.pyx:            sage: LF._element_constructor_(123)
src/sage/numerical/linear_functions.pyx:            sage: LC._element_constructor_(1, 2)
src/sage/numerical/linear_tensor_constraints.py:            sage: LTC._element_constructor_(1, 2, True)
src/sage/sets/disjoint_union_enumerated_sets.py:            sage: U._element_constructor_
src/sage/sets/disjoint_union_enumerated_sets.py:            sage: U._element_constructor_
src/sage/sets/disjoint_union_enumerated_sets.py:            sage: p = X._element_constructor_((0, []))  # indirect doctest
src/sage/sets/non_negative_integers.py:            sage: NN._element_constructor_(42)
src/sage/sets/non_negative_integers.py:            sage: NN._element_constructor_(-5)
src/sage/sets/non_negative_integers.py:            sage: NN._element_constructor_(x)
src/sage/groups/affine_gps/euclidean_group.py:            sage: E3._element_constructor_check(A, b)
src/sage/groups/affine_gps/euclidean_group.py:            sage: E3._element_constructor_check(A, b)
src/sage/groups/affine_gps/affine_group.py:            sage: Aff3._element_constructor_check(A, b)
src/sage/groups/affine_gps/affine_group.py:            sage: Aff3._element_constructor_check(A, b)
src/sage/rings/fraction_field.py:            sage: F._element_constructor_(1)
src/sage/rings/fraction_field.py:            sage: F._element_constructor_(F.gen(0)/F.gen(1))
src/sage/rings/fraction_field.py:            sage: F._element_constructor_('1 + x/y')
src/sage/rings/fraction_field.py:            sage: F._element_constructor_(x/y)
src/sage/rings/fraction_field.py:            sage: K._element_constructor_(2, 'x+y')
src/sage/rings/fraction_field.py:            sage: K._element_constructor_(1, 'z')
src/sage/rings/finite_rings/residue_field.pyx:            sage: ResidueField_generic._element_constructor_(F, i)
src/sage/rings/finite_rings/residue_field.pyx:            sage: ResidueField_generic._element_constructor_(F, GF(13)(8))
src/sage/rings/polynomial/plural.pyx:            sage: P._element_constructor_(1/2)
src/sage/rings/polynomial/plural.pyx:            sage: P._element_constructor_(x*y)
src/sage/rings/polynomial/plural.pyx:            sage: P._element_constructor_(y*x)
src/sage/rings/polynomial/plural.pyx:            sage: P._element_constructor_(1)
src/sage/rings/polynomial/plural.pyx:            sage: P._element_constructor_(0)
src/sage/rings/multi_power_series_ring.py:            sage: M._element_constructor_(m)
src/sage/rings/multi_power_series_ring.py:            sage: M._element_constructor_(p)
src/sage/rings/multi_power_series_ring.py:            sage: M._element_constructor_(p).parent()
src/sage/rings/function_field/function_field.py:            sage: L._element_constructor_(L.polynomial_ring().gen())
src/sage/rings/function_field/function_field.py:            sage: a = K._element_constructor_(K.maximal_order().gen()); a
src/sage/rings/function_field/function_field_order.py:            sage: K.maximal_order()._element_constructor_(x)
src/sage/rings/function_field/function_field_order.py:            sage: O._element_constructor_(y)
src/sage/rings/function_field/function_field_order.py:            sage: O._element_constructor_(1/y)
src/sage/combinat/rooted_tree.py:            sage: B._element_constructor_([])
src/sage/combinat/root_system/weyl_characters.py:            sage: A2._element_constructor_([2,1,0])
src/sage/combinat/root_system/weyl_characters.py:            sage: A2._element_constructor_([2,1,0])
src/sage/combinat/root_system/associahedron.py:            sage: parent._element_constructor_(['A',2])
src/sage/combinat/finite_class.py:            sage: F._element_constructor_(1)
src/sage/combinat/partition_tuple.py:            sage: PartitionTuples._element_constructor_(PartitionTuples(), [[2,1],[3,2],[1,1,1]])
src/sage/combinat/integer_lists/lists.py:            sage: L._element_constructor_nocheck([1,2,3])
src/sage/combinat/integer_lists/lists.py:            sage: L._element_constructor
src/sage/combinat/integer_lists/lists.py:            sage: L._element_constructor([1,2,3])
src/sage/combinat/binary_tree.py:            sage: B._element_constructor_([])
src/sage/combinat/binary_tree.py:            sage: FB._element_constructor_([])
src/sage/combinat/schubert_polynomial.py:            sage: X._element_constructor_([2,1,3])
src/sage/combinat/schubert_polynomial.py:            sage: X._element_constructor_(Permutation([2,1,3]))
src/sage/combinat/schubert_polynomial.py:            sage: X._element_constructor_([1,2,1])
src/sage/combinat/sf/classical.py:            sage: s._element_constructor_(McdJ(s[2,1]))
src/sage/combinat/posets/linear_extensions.py:            sage: x = L._element_constructor_([1,2,4,3]); x
src/sage/combinat/posets/linear_extensions.py:            sage: L._element_constructor_([4,3,2,1])
src/sage/combinat/posets/linear_extensions.py:            sage: L._element_constructor_([4,3,2,1],check=False)
src/sage/geometry/triangulation/point_configuration.py:            sage: p._element_constructor_([ (0,1,2), (2,3,0) ])
src/sage/geometry/linear_expression.py:            sage: L._element_constructor([1, 2, 3], 4)
src/sage/geometry/linear_expression.py:            sage: L._element_constructor(vector(ZZ, [1, 2, 3]), 4)
src/sage/geometry/linear_expression.py:            sage: L._element_constructor(4)
src/sage/geometry/linear_expression.py:            sage: L._element_constructor(vector([4, 1, 2, 3]))
src/sage/geometry/linear_expression.py:            sage: L._element_constructor(m)
src/sage/geometry/polyhedron/parent.py:            sage: P._element_constructor_([[(0, 0, 0), (1, 0, 0), (0, 1, 0), (0,0,1)], [], []], None)
src/sage/geometry/hyperplane_arrangement/hyperplane.py:        sage: ambient._element_constructor_(x+y-1)   
src/sage/geometry/hyperplane_arrangement/arrangement.py:            sage: L._element_constructor_(x, y)
src/sage/geometry/hyperplane_arrangement/arrangement.py:            sage: L._element_constructor_([x, y])
src/sage/geometry/hyperplane_arrangement/arrangement.py:            sage: L._element_constructor_([0, 1, 0], [0, 0, 1])
src/sage/geometry/hyperplane_arrangement/arrangement.py:            sage: L._element_constructor_([[0, 1, 0], [0, 0, 1]])
src/sage/geometry/hyperplane_arrangement/arrangement.py:            sage: L._element_constructor(polytopes.hypercube(2))
src/sage/homology/chain_complex.py:            sage: D._element_constructor_(0)
src/sage/tensor/modules/free_module_homset.py:            sage: phi = H._element_constructor_([[2,-1,3], [1,0,-4]], bases=(e,f),
src/sage/tensor/modules/free_module_homset.py:            sage: phi = EM._element_constructor_([[1,2,3],[4,5,6],[7,8,9]],
src/sage/tensor/modules/free_module_homset.py:            sage: phi_a = EM._element_constructor_(a) ; phi_a
src/sage/tensor/modules/ext_pow_free_module.py:            sage: a = A._element_constructor_(0) ; a
src/sage/tensor/modules/ext_pow_free_module.py:            sage: a = A._element_constructor_([], name='a') ; a
src/sage/tensor/modules/ext_pow_free_module.py:            sage: a = A._element_constructor_(0) ; a
src/sage/tensor/modules/ext_pow_free_module.py:            sage: a = A._element_constructor_([2,0,-1], name='a') ; a
src/sage/tensor/modules/ext_pow_free_module.py:            sage: a = A._element_constructor_(0) ; a
src/sage/tensor/modules/ext_pow_free_module.py:            sage: a = A._element_constructor_([], name='a') ; a
src/sage/tensor/modules/finite_rank_free_module.py:            sage: v = M._element_constructor_(comp=[1,0,-2], basis=e, name='v') ; v
src/sage/tensor/modules/finite_rank_free_module.py:            sage: v = M._element_constructor_(0) ; v
src/sage/tensor/modules/finite_rank_free_module.py:            sage: v = M._element_constructor_() ; v
src/sage/tensor/modules/free_module_linear_group.py:            sage: a = GL._element_constructor_(comp=[[1,2],[1,3]], basis=e,
src/sage/tensor/modules/free_module_linear_group.py:            sage: GL._element_constructor_(1)
src/sage/tensor/modules/free_module_linear_group.py:            sage: GL._element_constructor_(1).matrix(e)
src/sage/tensor/modules/free_module_linear_group.py:            sage: a = GL._element_constructor_(phi) ; a
src/sage/tensor/modules/free_module_linear_group.py:            sage: a = GL._element_constructor_(t) ; a
src/sage/tensor/modules/tensor_free_module.py:            sage: T._element_constructor_(0) is T.zero()
src/sage/tensor/modules/tensor_free_module.py:            sage: t = T._element_constructor_(comp=[[2,0],[1/2,-3]], basis=e,
src/sage/categories/sets_cat.py:                sage: S._element_constructor_(17)
src/sage/categories/sets_cat.py:                sage: A._element_constructor
src/sage/categories/sets_cat.py:                sage: B._element_constructor
src/sage/categories/sets_cat.py:                sage: s = S._element_constructor_from_element_class(17); s
src/sage/categories/examples/semigroups.py:            sage: F._element_constructor_('a')
src/sage/categories/examples/semigroups.py:            sage: F._element_constructor_('bad')
src/sage/categories/examples/semigroups.py:            sage: F._element_constructor_('z')
src/sage/categories/examples/semigroups.py:            sage: bad = F._element_constructor_('bad'); bad
src/sage/categories/examples/semigroups.py:            sage: type(S._element_constructor_(17))
src/sage/categories/examples/semigroups.py:            sage: S._element_constructor_(17)
src/sage/categories/examples/semigroups.py:            sage: type(S._element_constructor_(17))
src/sage/categories/examples/sets_cat.py:            sage: P._element_constructor_(13) == 13
src/sage/categories/examples/sets_cat.py:            sage: P._element_constructor_(13).parent()
src/sage/categories/examples/sets_cat.py:            sage: P._element_constructor_(14)
src/sage/categories/examples/sets_cat.py:            sage: P._element_constructor_(14)
src/sage/categories/examples/sets_cat.py:            sage: P._element_constructor_(14)
src/sage/structure/parent.pyx:            sage: k = GF(5); k._element_constructor # indirect doctest
src/sage/structure/set_factories.py:            sage: pol.self_element_constructor_attributes(XYPair)
src/sage/structure/set_factories.py:            sage: pol.facade_element_constructor_attributes(XYPairs())
src/sage/manifolds/differentiable/real_line.py:            sage: I((2,))  # standard used of TopologicalManifoldSubset._element_constructor_
src/sage/algebras/steenrod/steenrod_algebra.py:            sage: A1._element_constructor_(Sq(2))
src/sage/algebras/steenrod/steenrod_algebra.py:            sage: A1._element_constructor_(Sq(4)) # Sq(4) not in A1
src/sage/algebras/free_algebra_quotient.py:            sage: H._element_constructor_(i) is i
src/sage/algebras/free_algebra_quotient.py:            sage: a = H._element_constructor_(1); a
src/sage/algebras/free_algebra_quotient.py:            sage: a = H._element_constructor_([1,2,3,4]); a

Of course, the documentation should explain how to implement _element_constructor_. However, the documentation must not suggest that the method (with or without trailing underscore) should ever be called directly.

[jdemeyer]

Branch: u/jdemeyer/rename_parent__element_constructor____parent___construct_element

[koffie]

New commits:

e9ce136	_element_constructor was an attribute of Subword before it was replaced with _build
93e25fb	Rename Parent._element_constructor -> Parent.__construct_element

[koffie]

Commit: 93e25fb

[simon-king-jena]

Changed branch from u/jdemeyer/rename_parent__element_constructor____parent___construct_element to u/SimonKing/rename_parent__element_constructor____parent___construct_element

[simon-king-jena]

comment:22

Briefly looking at your commit: I find it a good idea that you are using a cdef __construct_element instead of a cdef public _element_constructor, because in that way it is certain that no Python class will illegally use the internal element constructor. I also see that you make pickling backward compatible.

My commit fixes one part of the documentation (in sage.schemes).

---

New commits:

c95b724

Remove ._element_constructor_ from sage.schemes documentation

[simon-king-jena]

Changed commit from 93e25fb to c95b724

[simon-king-jena]

Changed author from Jeroen Demeyer to Jeroen Demeyer, Simon King

[jdemeyer]

Changed dependencies from #23882 to #23882, #23884

[jdemeyer]

New commits:

6878e67	Avoid _element_constructor in padics
44bc9fb	Do not assign _element_constructor in combinat/sf
f684cb0	Implement `_element_constructor_` for Homset
8b9765e	Avoid _element_constructor in geometry doctests
2630849	Various minor changes to _element_constructor
2856051	Merge commit '6878e67e99f1f3707c8b4a060264f5484989c52b'; commit '44bc9fb660fe4508b0bd8340667b8b65b70226a1'; commit 'f684cb0e4834a0041da29ef7a1f8f0b01cf84994'; commit '8b9765e781f8f9359cfc6021f6cd3d0e3b4b36d6'; commit '2630849a988376b44769460989a41919bbe89520' into t/23881/rename_parent__element_constructor____parent___construct_element
77ccb85	Do not set _element_init_pass_parent
41614a7	Remove Python access from Parent._element_constructor and Parent._element_init_pass_parent

[tscrim]

comment:51

Green patchbot => positive review.

[tscrim]

Reviewer: Travis Scrimshaw

[koffie]

comment:52

Both arando and rk02-math seem to fail the same test:

 sage -t --long src/sage/schemes/elliptic_curves/sha_tate.py # 1 doctest failed 

Which they do not seem to fail on with other tickets.

[tscrim]

comment:53

I don't see how those failures could be related to the changes in this ticket unless they appear also in one of the dependencies. Even still, I don't see how they could be connected, much less be machine dependent.

[koffie]

comment:54

I don't see see why either so I agree it is highly unlikely, but since parent is modified it is not impossible. It does not need to be machine dependent, it can also be a bug that happens 50% of the time that is exposed after this.

There are two machines failing on this ticket and I have not seen it on either arando or rk02-math in at least 10 other failing logs that I checked. So from this point of view it is also very unlikely that it is not from this ticket. Of course this ticket could have been very unlucky to expose a rare random bug twice for the first time but this is also highly unlikely.

I just would like to see arguments for why we are in one unlikely scenario and not the other.

Note that this ticket also has a lot of dependencies merged so it could also be exposed by one of the dependencies, which might explain why I didn't see it on any of the arando and rk02-math logsI checked.

If only we could search through the patchbot log easily, then we could get conclusive answers much faster.

[jdemeyer]

comment:55

I don't think that the failures have anything to do with this ticket, so I'm setting it to positive_review. If the failures are real, then it will fail on Volker's buildbot anyway.

[jdemeyer]

comment:56

I also tested it on arando manually (without the patchbot) and that test passed.

[koffie]

comment:57

Before you reported that arando passes all test I kicked the patchbot to get more info. And arando did fail again on the run resulting from that kick, while the bug did not occur in two tests ran on arando in between.

I am a bit mystified by the situation since it can't be the optional packages since rk02-math has none, so it must be a randomly occuring bug.

[koffie]

comment:58

I tested this with

./sage -t --long --global-iterations=100 src/sage/schemes/elliptic_curves/sha_tate.py

on arando and they all pass so it is not easily reproducible.

[koffie]

comment:59

The sha_tate doctest fails on sage8.1.beta7 without any patches on arando so I created #23962, since arando failed on the 3 tests it ran on this ticket, but none of the other beta6 tests, I still suspect it to be something introduced by one of the dependencies of this ticket that got merged in sage8.1.beta7. Anyway it can't be this ticket so lets move the discussion to #23962.

[jdemeyer]

comment:60

Replying to @tscrim:

to prevent anyone else from adding something (although I would say that is highly unlikely).

It seems that precisely this has happened in #23660...

[jdemeyer]

Changed dependencies from #23882, #23884, #23894, #23899, #23905, #23907, #23914 to #23882, #23884, #23894, #23899, #23905, #23907, #23914, #24033

[vbraun]

Changed branch from u/jdemeyer/rename_parent__element_constructor____parent___construct_element to 41614a7

[jdemeyer]

comment:63

Good that this is merged now...

[jdemeyer]

Changed commit from 41614a7 to none