Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#23881 closed defect (fixed)

Remove Python access from Parent._element_constructor

Reported by: jdemeyer Owned by:
Priority: trivial Milestone: sage-8.1
Component: coercion Keywords:
Cc: SimonKing, mderickx Merged in:
Authors: Jeroen Demeyer, Simon King Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 41614a7 (Commits, GitHub, GitLab) Commit:
Dependencies: #23882, #23884, #23894, #23899, #23905, #23907, #23914, #24033 Stopgaps:

Status badges

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

Change History (63)

comment:1 in reply to: ↑ description ; follow-up: Changed 4 years ago by SimonKing

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.

comment:2 in reply to: ↑ 1 ; follow-up: Changed 4 years ago by jdemeyer

Replying to SimonKing:

I am not sure whether _element_constructor is needed at all.

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

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

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

Replying to jdemeyer:

Replying to SimonKing:

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.

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

Replying to SimonKing:

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.

comment:5 follow-ups: Changed 4 years ago by SimonKing

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()

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

Replying to SimonKing:

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

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

comment:7 in reply to: ↑ 5 ; follow-ups: Changed 4 years ago by SimonKing

Replying to SimonKing:

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

Why should it be a tuple?? Very strange.

comment:8 in reply to: ↑ 7 Changed 4 years ago by SimonKing

Replying to SimonKing:

Replying to SimonKing:

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

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

comment:9 in reply to: ↑ 7 ; follow-up: Changed 4 years ago by mantepse

Replying to SimonKing:

Replying to SimonKing:

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 https://git.sagemath.org/sage.git/commit/?id=a2915642f7f5357e0d374aaf007680c263cd164c maybe that should have been self._build

comment:10 in reply to: ↑ 9 ; follow-up: Changed 4 years ago by SimonKing

Replying to mantepse:

looking at the last few lines of https://git.sagemath.org/sage.git/commit/?id=a2915642f7f5357e0d374aaf007680c263cd164c 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.

comment:11 in reply to: ↑ 10 ; follow-up: Changed 4 years ago by mantepse

Replying to SimonKing:

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

comment:12 in reply to: ↑ 11 ; follow-up: Changed 4 years ago by SimonKing

Replying to mantepse:

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

What is __iter_new__?

comment:13 in reply to: ↑ 12 Changed 4 years ago by mantepse

Replying to SimonKing:

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.

comment:14 Changed 4 years ago by SimonKing

  • Dependencies set to #23882

comment:15 Changed 4 years ago by SimonKing

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

comment:16 Changed 4 years ago by mantepse

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

comment:17 Changed 4 years ago by mantepse

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

comment:18 Changed 4 years ago by SimonKing

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.

comment:19 Changed 4 years ago by jdemeyer

  • Branch set to u/jdemeyer/rename_parent__element_constructor____parent___construct_element

comment:20 Changed 4 years ago by mderickx

  • Cc mderickx added
  • Commit set to 93e25fb2489c5a98c8ec2a018d23754987ce6937

New commits:

e9ce136_element_constructor was an attribute of Subword before it was replaced with _build
93e25fbRename Parent._element_constructor -> Parent.__construct_element

comment:21 Changed 4 years ago by SimonKing

  • Branch changed from u/jdemeyer/rename_parent__element_constructor____parent___construct_element to u/SimonKing/rename_parent__element_constructor____parent___construct_element

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

  • Commit changed from 93e25fb2489c5a98c8ec2a018d23754987ce6937 to c95b72453d16b13feff90c0308cdec5b3e4c5ac0

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:

c95b724Remove ._element_constructor_ from sage.schemes documentation

comment:23 Changed 4 years ago by SimonKing

  • Authors changed from Jeroen Demeyer to Jeroen Demeyer, Simon King

comment:24 Changed 4 years ago by jdemeyer

  • Dependencies changed from #23882 to #23882, #23884

comment:25 follow-up: Changed 4 years ago by SimonKing

So far, the git commits come from Maarten and me, so, I guess the Authors field should be changed accordingly.

On a different ticket, Jeroen said that he likes small tickets. I guess that means I should open a new ticket for cleanup of the documentation.

I suggest, in order to not change things in git that have already been done, to let commit:c95b724 (Remove ._element_constructor_ from sage.schemes documentation) stay here and open a new ticket for other documentation issues.

First of all, the failures resulting from Maarten's commit should be addressed.

comment:26 in reply to: ↑ 25 ; follow-ups: Changed 4 years ago by SimonKing

Replying to SimonKing:

First of all, the failures resulting from Maarten's commit should be addressed.

Nope. Some failures come from padics. So, first #23884 needs to be fixed. That's why I don't like creating many small tickets.

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

Replying to SimonKing:

Replying to SimonKing:

First of all, the failures resulting from Maarten's commit should be addressed.

Nope. Some failures come from padics. So, first #23884 needs to be fixed. That's why I don't like creating many small tickets.

Anyway, if we are lucky, merging Jeroen's branch from #23884 will fix the issue. I am trying that now.

comment:28 Changed 4 years ago by mantepse

For the doc of binary trees, it is #23885

comment:29 Changed 4 years ago by mantepse

For the remainder of combinat, it is #23889. However, I'm not so sure about this anymore: _element_constructor_ is a private method, so it won't show up in the documentation anyway? So, isn't it correct to doctest it directly?

comment:30 in reply to: ↑ 26 Changed 4 years ago by jdemeyer

Replying to SimonKing:

Some failures come from padics. So, first #23884 needs to be fixed. That's why I don't like creating many small tickets.

I don't really get this. The padic failures need to be fixed anyway, either here or on #23884. Why don't you like to do it on a different ticket?

comment:31 in reply to: ↑ 26 Changed 4 years ago by jdemeyer

Replying to SimonKing:

That's why I don't like creating many small tickets.

Interestingly, you are the first person I am hearing this from.

Advantages of small tickets:

  1. Small tickets are easier to review because the reviewer only needs to look at a small set of changes.
  1. Different tickets can have different sets of authors/reviewers, each with their own interests/specialities.
  1. Small tickets are less likely to get stuck because of one isolated difficulty. For example: a ticket like "stop using _element_constructor" cannot succeed as long as there is one isolated place where _element_constructor is used non-trivially. A ticket like "stop using _element_constructor in sub-package foo" does not have this problem.

Usually, when working on a big ticket like #23880 (big in the sense that it has a lot of connections to other parts of Sage, it is very non-local), I start coding and then split off separate tickets as I go along.

comment:32 follow-up: Changed 4 years ago by mantepse

I'd be genuinly interested: shouldn't doctests of a private method like _element_constructor_ better doctest this method directly? Or should I ask this on sage-devel?

comment:33 in reply to: ↑ 32 ; follow-up: Changed 4 years ago by jdemeyer

Replying to mantepse:

I'd be genuinly interested: shouldn't doctests of a private method like _element_constructor_ better doctest this method directly? Or should I ask this on sage-devel?

In the end, both should be tested: direct and indirect tests. In most cases, the indirect usage is already tested (many doctests indirectly use _element_constructor_). So, yes, I think that _element_constructor_ should be tested directly.

comment:34 in reply to: ↑ 33 ; follow-ups: Changed 4 years ago by SimonKing

Replying to jdemeyer:

Replying to mantepse:

I'd be genuinly interested: shouldn't doctests of a private method like _element_constructor_ better doctest this method directly? Or should I ask this on sage-devel?

In the end, both should be tested: direct and indirect tests. In most cases, the indirect usage is already tested (many doctests indirectly use _element_constructor_). So, yes, I think that _element_constructor_ should be tested directly.

Seriously??? When I was younger, I occasionally did direct tests (say, for __iter__) and was told to not do that.

I also think it has been discussed on sage-devel a long time ago and the consensus was that the documentation (in particular the "EXAMPLES:" section) has to show how stuff is actually being used. It may be possible to be a bit more explicit in the "TESTS:" section, but still I'd recommend against it.

It is clear from the tutorials that _repr_, _add_, _mul_ and _element_constructor_ (all single underscore!) in Sage implement string representation of parents/elements, addition resp. multiplication of elements, and the call method of parents. Thus, all these methods should be tested indirectly:

sage: P    # indirect doctest
String representation
sage: x+y  # indirect doctest
result of addition
sage: P(x) # indirect doctest
result of conversion
Last edited 4 years ago by SimonKing (previous) (diff)

comment:35 in reply to: ↑ 34 Changed 4 years ago by SimonKing

Replying to SimonKing:

I also think it has been discussed on sage-devel a long time ago and the consensus was that the documentation (in particular the "EXAMPLES:" section) has to show how stuff is actually being used. It may be possible to be a bit more explicit in the "TESTS:" section, but still I'd recommend against it.

I tried to find the discussion, but didn't succeed. So, if you prefer, do ask on Sage-devel.

comment:36 in reply to: ↑ 34 ; follow-up: Changed 4 years ago by jdemeyer

Replying to SimonKing:

Seriously??? When I was younger, I occasionally did direct tests (say, for __iter__) and was told to not do that.

I think Python special methods like __iter__ and private methods like _element_constructor_ are a different category. This is especially so for a cdef class, where __iter__ is not a method at all! So whatever you think about __iter__ may not apply to _element_constructor_.

I also think it has been discussed on sage-devel a long time ago and the consensus was that the documentation (in particular the "EXAMPLES:" section) has to show how stuff is actually being used.

Well, _element_constructor_ is not actually used by the end user. So again, this is not a relevant point.

In the end I would say that it doesn't really matter. Maybe add both kinds of tests to be safe?

comment:37 in reply to: ↑ 36 ; follow-up: Changed 4 years ago by mantepse

In the end I would say that it doesn't really matter. Maybe add both kinds of tests to be safe?

Well, currently we are removing these tests. So if we decide here that it's not bad practice to test _element_constructor_ directly, several of the tickets can be closed immediately.

Personally, I think that indirect doctests do not add much value and are hard to write. So, I think we could close all these tickets again - I'd move out the extra stuff from #23885. But I don't care much, comment:34 is convincing, too. Actually, I didn't know that _element_constructor_ always invokes the call method of the parent.

Anyway, let's not waste time on this.

comment:38 Changed 4 years ago by jdemeyer

  • Dependencies changed from #23882, #23884 to #23882, #23884, #23894

comment:39 in reply to: ↑ 37 ; follow-up: Changed 4 years ago by tscrim

Replying to mantepse:

In the end I would say that it doesn't really matter. Maybe add both kinds of tests to be safe?

Well, currently we are removing these tests. So if we decide here that it's not bad practice to test _element_constructor_ directly, several of the tickets can be closed immediately.

I am a strong -1 on removing these tests. There can be good reasons for calling it directly, such as having non-trivial parsing of inputs before sending it to the element class but wanting to avoid the coercion framework. I also cannot see any problems with a direct call. Since this is being tested directly in the corresponding method, it is not public, and so it won't cause any problems for users.

Personally, I think that indirect doctests do not add much value and are hard to write. So, I think we could close all these tickets again - I'd move out the extra stuff from #23885. But I don't care much, comment:34 is convincing, too.

Do you mean comment:33 or comment:36?

I usually am just lazy and do indirect for _element_constructor_ tests. However, I probably should do direct calls in tests... However, for some of the methods, it is just easier (and can be more readable, such as for algebraic operations) to do the indirect test.

Actually, I didn't know that _element_constructor_ always invokes the call method of the parent.

It is the other way around; the _element_constructor_ is called from Parent.__call__. It usually is the last thing called from the parent and actually constructs the element when doing Foo(bar).

comment:40 in reply to: ↑ 39 Changed 4 years ago by mantepse

I am a strong -1 on removing these tests.

OK - so please let's ignore these tickets for the moment and concentrate on this one and #23880 instead...

Personally, I think that indirect doctests do not add much value and are hard to write. So, I think we could close all these tickets again - I'd move out the extra stuff from #23885. But I don't care much, comment:34 is convincing, too.

Do you mean comment:33 or comment:36?

No, I meant comment:34. Specifically:

It is clear from the tutorials that _repr_, _add_, _mul_ and _element_constructor_ (all single underscore!) in Sage implement string representation of parents/elements, addition resp. multiplication of elements, and the call method of parents. Thus, all these methods should be tested indirectly:

sage: P    # indirect doctest
String representation
sage: x+y  # indirect doctest
result of addition
sage: P(x) # indirect doctest
result of conversion

Actually, I didn't know that _element_constructor_ always invokes the call method of the parent.

It is the other way around; the _element_constructor_ is called from Parent.__call__. It usually is the last thing called from the parent and actually constructs the element when doing Foo(bar).

Thanks for the clarification!

comment:41 Changed 4 years ago by jdemeyer

  • Dependencies changed from #23882, #23884, #23894 to #23882, #23884, #23894, #23899

comment:42 Changed 4 years ago by jdemeyer

  • Dependencies changed from #23882, #23884, #23894, #23899 to #23882, #23884, #23894, #23899, #23905

comment:43 Changed 4 years ago by jdemeyer

  • Dependencies changed from #23882, #23884, #23894, #23899, #23905 to #23882, #23884, #23894, #23899, #23905, #23907

comment:44 Changed 4 years ago by jdemeyer

  • Dependencies changed from #23882, #23884, #23894, #23899, #23905, #23907 to #23882, #23884, #23894, #23899, #23905, #23907, #23914

comment:45 in reply to: ↑ 22 Changed 4 years ago by jdemeyer

With all the dependencies on this ticket and #23880, I am no longer convinced that we need this ticket. We could also simply do the change cdef public _element_constructor -> cdef _element_constructor here and nothing else.

comment:46 follow-up: Changed 4 years ago by tscrim

I think removing _element_constructor from the public Python API is a good thing to do to prevent anyone else from adding something (although I would say that is highly unlikely).

comment:47 Changed 4 years ago by jdemeyer

  • Description modified (diff)
  • Summary changed from Rename Parent._element_constructor -> Parent.__construct_element to Remove Python access from Parent._element_constructor

comment:48 Changed 4 years ago by jdemeyer

  • Description modified (diff)

comment:49 Changed 4 years ago by jdemeyer

  • Branch changed from u/SimonKing/rename_parent__element_constructor____parent___construct_element to u/jdemeyer/rename_parent__element_constructor____parent___construct_element

comment:50 Changed 4 years ago by jdemeyer

  • Commit changed from c95b72453d16b13feff90c0308cdec5b3e4c5ac0 to 41614a7ea0c82da579a039eecd51ee628a3e41e0
  • Status changed from new to needs_review

New commits:

6878e67Avoid _element_constructor in padics
44bc9fbDo not assign _element_constructor in combinat/sf
f684cb0Implement _element_constructor_ for Homset
8b9765eAvoid _element_constructor in geometry doctests
2630849Various minor changes to _element_constructor
2856051Merge commit '6878e67e99f1f3707c8b4a060264f5484989c52b'; commit '44bc9fb660fe4508b0bd8340667b8b65b70226a1'; commit 'f684cb0e4834a0041da29ef7a1f8f0b01cf84994'; commit '8b9765e781f8f9359cfc6021f6cd3d0e3b4b36d6'; commit '2630849a988376b44769460989a41919bbe89520' into t/23881/rename_parent__element_constructor____parent___construct_element
77ccb85Do not set _element_init_pass_parent
41614a7Remove Python access from Parent._element_constructor and Parent._element_init_pass_parent

comment:51 Changed 4 years ago by tscrim

  • Reviewers set to Travis Scrimshaw

Green patchbot => positive review.

comment:52 Changed 4 years ago by mderickx

  • Status changed from needs_review to needs_work

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.

comment:53 Changed 4 years ago by tscrim

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.

comment:54 Changed 4 years ago by mderickx

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.

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

comment:55 Changed 4 years ago by jdemeyer

  • Status changed from needs_work to positive_review

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.

comment:56 Changed 4 years ago by jdemeyer

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

comment:57 Changed 4 years ago by mderickx

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.

comment:58 Changed 4 years ago by mderickx

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.

comment:59 Changed 4 years ago by mderickx

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.

comment:60 in reply to: ↑ 46 Changed 4 years ago by jdemeyer

  • Status changed from positive_review to needs_work

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

comment:61 Changed 4 years ago by jdemeyer

  • Dependencies changed from #23882, #23884, #23894, #23899, #23905, #23907, #23914 to #23882, #23884, #23894, #23899, #23905, #23907, #23914, #24033
  • Status changed from needs_work to positive_review

comment:62 Changed 4 years ago by vbraun

  • Branch changed from u/jdemeyer/rename_parent__element_constructor____parent___construct_element to 41614a7ea0c82da579a039eecd51ee628a3e41e0
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:63 Changed 4 years ago by jdemeyer

  • Commit 41614a7ea0c82da579a039eecd51ee628a3e41e0 deleted

Good that this is merged now...

Note: See TracTickets for help on using tickets.