#23881 closed defect (fixed)
Remove Python access from Parent._element_constructor
Reported by:  jdemeyer  Owned by:  

Priority:  trivial  Milestone:  sage8.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: 
Description (last modified by )
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 ; followup: ↓ 2 Changed 4 years ago by
comment:2 in reply to: ↑ 1 ; followup: ↓ 3 Changed 4 years ago by
comment:3 in reply to: ↑ 2 ; followup: ↓ 4 Changed 4 years ago by
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
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 followups: ↓ 6 ↓ 7 Changed 4 years ago by
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
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.
comment:7 in reply to: ↑ 5 ; followups: ↓ 8 ↓ 9 Changed 4 years ago by
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
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 slowdown, 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 ; followup: ↓ 10 Changed 4 years ago by
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 ; followup: ↓ 11 Changed 4 years ago by
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 ; followup: ↓ 12 Changed 4 years ago by
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 ; followup: ↓ 13 Changed 4 years ago by
comment:13 in reply to: ↑ 12 Changed 4 years ago by
comment:14 Changed 4 years ago by
 Dependencies set to #23882
comment:15 Changed 4 years ago by
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
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
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
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+y1) 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
 Branch set to u/jdemeyer/rename_parent__element_constructor____parent___construct_element
comment:20 Changed 4 years ago by
 Cc mderickx added
 Commit set to 93e25fb2489c5a98c8ec2a018d23754987ce6937
comment:21 Changed 4 years ago by
 Branch changed from u/jdemeyer/rename_parent__element_constructor____parent___construct_element to u/SimonKing/rename_parent__element_constructor____parent___construct_element
comment:22 followup: ↓ 45 Changed 4 years ago by
 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:
c95b724  Remove ._element_constructor_ from sage.schemes documentation

comment:23 Changed 4 years ago by
comment:24 Changed 4 years ago by
 Dependencies changed from #23882 to #23882, #23884
comment:25 followup: ↓ 26 Changed 4 years ago by
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 ; followups: ↓ 27 ↓ 30 ↓ 31 Changed 4 years ago by
comment:27 in reply to: ↑ 26 Changed 4 years ago by
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
For the doc of binary trees, it is #23885
comment:29 Changed 4 years ago by
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
comment:31 in reply to: ↑ 26 Changed 4 years ago by
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:
 Small tickets are easier to review because the reviewer only needs to look at a small set of changes.
 Different tickets can have different sets of authors/reviewers, each with their own interests/specialities.
 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 nontrivially. A ticket like "stop using_element_constructor
in subpackage 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 nonlocal), I start coding and then split off separate tickets as I go along.
comment:32 followup: ↓ 33 Changed 4 years ago by
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 sagedevel?
comment:33 in reply to: ↑ 32 ; followup: ↓ 34 Changed 4 years ago by
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 sagedevel?
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 ; followups: ↓ 35 ↓ 36 Changed 4 years ago by
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 sagedevel?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 sagedevel 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
comment:35 in reply to: ↑ 34 Changed 4 years ago by
Replying to SimonKing:
I also think it has been discussed on sagedevel 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 Sagedevel.
comment:36 in reply to: ↑ 34 ; followup: ↓ 37 Changed 4 years ago by
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 sagedevel 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 ; followup: ↓ 39 Changed 4 years ago by
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
 Dependencies changed from #23882, #23884 to #23882, #23884, #23894
comment:39 in reply to: ↑ 37 ; followup: ↓ 40 Changed 4 years ago by
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 nontrivial 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
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 fromParent.__call__
. It usually is the last thing called from the parent and actually constructs the element when doingFoo(bar)
.
Thanks for the clarification!
comment:41 Changed 4 years ago by
 Dependencies changed from #23882, #23884, #23894 to #23882, #23884, #23894, #23899
comment:42 Changed 4 years ago by
 Dependencies changed from #23882, #23884, #23894, #23899 to #23882, #23884, #23894, #23899, #23905
comment:43 Changed 4 years ago by
 Dependencies changed from #23882, #23884, #23894, #23899, #23905 to #23882, #23884, #23894, #23899, #23905, #23907
comment:44 Changed 4 years ago by
 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
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 followup: ↓ 60 Changed 4 years ago by
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
 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
 Description modified (diff)
comment:49 Changed 4 years ago by
 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
 Commit changed from c95b72453d16b13feff90c0308cdec5b3e4c5ac0 to 41614a7ea0c82da579a039eecd51ee628a3e41e0
 Status changed from new to needs_review
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

comment:51 Changed 4 years ago by
 Reviewers set to Travis Scrimshaw
Green patchbot => positive review.
comment:52 Changed 4 years ago by
 Status changed from needs_review to needs_work
Both arando and rk02math 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
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
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 rk02math 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 rk02math logsI checked.
If only we could search through the patchbot log easily, then we could get conclusive answers much faster.
comment:55 Changed 4 years ago by
 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
I also tested it on arando manually (without the patchbot) and that test passed.
comment:57 Changed 4 years ago by
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 rk02math has none, so it must be a randomly occuring bug.
comment:58 Changed 4 years ago by
I tested this with
./sage t long globaliterations=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
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
 Status changed from positive_review to needs_work
comment:61 Changed 4 years ago by
 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
 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
 Commit 41614a7ea0c82da579a039eecd51ee628a3e41e0 deleted
Good that this is merged now...
Replying to jdemeyer:
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
.