Opened 18 months ago

Last modified 10 days ago

#29101 needs_info defect

Refined protocol for _element_constructor_ in element classes with mutability

Reported by: nailuj Owned by:
Priority: major Milestone: sage-9.5
Component: misc Keywords: vector, constructor, copy
Cc: gh-kliem, gh-mjungmath, mkoeppe, tscrim, nbruin, dcoudert, gh-mwageringel, pbruin Merged in:
Authors: Reviewers:
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description

Given a vector v with base ring R, the constructions

  • w = vector(R, v)
  • w = vector(v, R)
  • w = vector(v)

all return the original vector v. As a consequence, all changes applied to w are also applied to v and vice versa.

sage: v = vector([1,2,3])
sage: w = vector(v, ZZ)
sage: w is v
True
sage: w[1] = 7
sage: v
(1, 7, 3)

Is this supposed to happen?

Change History (20)

comment:1 Changed 18 months ago by nailuj

Some examples for comparison:

For basic python types, similar constructions return

  • a new object, if it's mutable (list, set, dict)
    sage: L = [1,2,3]
    sage: M = list(L)
    sage: L is M
    False
    sage: S = {1,2,3}
    sage: T = set(S)
    sage: S is T
    False
    sage: D = dict({1:2, 3:4})
    sage: E = dict(D)
    sage: D is E
    False
    
  • the old object, if it's immutable (tuple)
    sage: X = (1,2,3)
    sage: Y = tuple(X)
    sage: X is Y
    True
    

Regarding two examples of Sage objects,

  • in Graphs, a new object is returned, whether the old one was set immutable or not
    sage: G = graphs.CompleteGraph(5)
    sage: H = Graph(G)
    sage: H is G
    False
    
  • in Sets, which wrap around an immutable frozenset, the old object is returned
    sage: S = Set([1,2,3])
    sage: T = Set(S)
    sage: S is T
    True
    

comment:2 Changed 18 months ago by nailuj

  • Status changed from new to needs_info

comment:3 Changed 15 months ago by mkoeppe

  • Milestone changed from sage-9.1 to sage-9.2

Moving tickets to milestone sage-9.2 based on a review of last modification date, branch status, and severity.

comment:4 Changed 12 months ago by mkoeppe

  • Cc gh-mjungmath added

comment:5 Changed 12 months ago by gh-mjungmath

We have a similar discussion running for FiniteRankFreeModule and elements in the manifolds module, take a look at #30302, especially comment:3.

In my opinion, this should not happen.

comment:6 Changed 12 months ago by gh-mjungmath

  • Cc mkoeppe tscrim added

Even though the corresponding check via the flac copy is implemented in FreeModule, the coercion model preempts it. The corresponding change should be

-        if R is self and no_extra_args:
-            return x

in sage.structure.parent.Parent.

But I don't know anything about the whole string of consequences coming from this.

Last edited 12 months ago by gh-mjungmath (previous) (diff)

comment:7 Changed 12 months ago by gh-mjungmath

Just for fun, I've deleted these lines and ran a doctest:

File "src/sage/structure/parent.pyx", line 1473, in sage.structure.parent.Parent._is_valid_homomorphism_._is_coercion_cached
Failed example:
    R._is_coercion_cached(QQ)
Expected:
    False
Got:
    True

comment:8 follow-up: Changed 12 months ago by mkoeppe

Note that FreeModule_generic._element_constructor_ has some optional arguments...

    def _element_constructor_(self, x, coerce=True, copy=True, check=True):

comment:9 in reply to: ↑ 8 Changed 12 months ago by gh-mjungmath

Replying to mkoeppe:

Note that FreeModule_generic._element_constructor_ has some optional arguments...

    def _element_constructor_(self, x, coerce=True, copy=True, check=True):

Regarding the default copy=True argument, a copy should be returned. But it is not due to the coercion framework.

comment:10 Changed 12 months ago by mkoeppe

Right. So how about changing this default to False? If one really needs a copy (which is currently not guaranteed, as you have observed), then one would call it with copy=True. I think (haven't tested!) that this will not go through coercion because this path is only taken for 1-argument calls of the parent.

comment:11 follow-up: Changed 12 months ago by gh-mjungmath

This could only be a temporary solution. As you already pointed out, and I totally agree with you, the element constructor should behave consistently, i.e. always returning a mutable element. Returning the very same instant, assuming it is immutable, would contradict this behavior.

comment:12 in reply to: ↑ 11 ; follow-up: Changed 12 months ago by mkoeppe

  • Summary changed from New vector from old vector returns the same object to Refined protocol for _element_constructor_ in element classes with mutability

Replying to gh-mjungmath:

As you already pointed out, and I totally agree with you, the element constructor should behave consistently, i.e. always returning a mutable element. Returning the very same instant, assuming it is immutable, would contradict this behavior.

I don't think this is my position; if I did say this, let me try to refine it here.

In https://trac.sagemath.org/ticket/30302#comment:3 I pointed out that arithmetic operations need to be consistent: Either always return a new element (even for trivial operations), or always return an immutable element.

It is not true that the parent object "is" the element constructor. Its __call__ methods serves several purposes: In one-argument calls, (1) it is the identity (in the strong sense of Python's id) on elements of its parent and (2) in general, a convert map, with (3) input 0 as a permissive special case, which is also the default argument of Parent.__call__. (4) In multiple-argument calls, it is the element constructor.

My proposal is to define a general protocol for element classes with mutability which does NOT change the above but only refines it for mutable classes as follows:

  • In element classes with mutability, _element_constructor_(x, ...) MUST support optional arguments copy and mutable.
  • These arguments are allowed to have various defaulting behavior that is the most appropriate for the specific class, but there are some restrictions:
  • copy MUST NOT default to True for mutable inputs x because (as discussed) this is not compatible with (1).
  • mutable=False and copy=False MUST be an error for mutable input x.

We could add _test_... methods that check this protocol.

comment:13 Changed 12 months ago by mkoeppe

  • Cc nbruin added

comment:14 Changed 12 months ago by mkoeppe

  • Cc dcoudert added

comment:15 Changed 12 months ago by mkoeppe

  • Cc gh-mwageringel added

comment:16 in reply to: ↑ 12 Changed 12 months ago by gh-mjungmath

Replying to mkoeppe:

Replying to gh-mjungmath:

As you already pointed out, and I totally agree with you, the element constructor should behave consistently, i.e. always returning a mutable element. Returning the very same instant, assuming it is immutable, would contradict this behavior.

I don't think this is my position; if I did say this, let me try to refine it here.

In https://trac.sagemath.org/ticket/30302#comment:3 I pointed out that arithmetic operations need to be consistent: Either always return a new element (even for trivial operations), or always return an immutable element.

I was more referring to your first paragraph:

Both M() and M(0) return a new mutable element. That's how one creates a new vector, whose components can then be modified. If you want the immutable 0 element, you can use M.zero().

Consider the following code:

sage: M = FreeModule(QQ, 3)
sage: v = M([1,2,3])
sage: v.set_immutable()
sage: M(v).is_immutable()
True

I don't see a point if the element constructor sometimes returns a mutable and sometimes an immutable element. This is how I understood your argument that we should M(0) return a new mutable "zero" istead of the same instance as M.zero(). If it really doesn't matter whether the element is mutable or not, we should allow M(0) returning the cached M.zero() instance since this is much much faster (especially in the manifold setting).

I'd say, if M a priori creates mutable elements, the element constructor should always return mutable elements. This would be consistent.

Last edited 12 months ago by gh-mjungmath (previous) (diff)

comment:17 Changed 11 months ago by mkoeppe

  • Milestone changed from sage-9.2 to sage-9.3

comment:18 Changed 7 months ago by mkoeppe

  • Cc pbruin added

comment:19 Changed 4 months ago by mkoeppe

  • Milestone changed from sage-9.3 to sage-9.4

Setting new milestone based on a cursory review of ticket status, priority, and last modification date.

comment:20 Changed 10 days ago by mkoeppe

  • Milestone changed from sage-9.4 to sage-9.5

Setting a new milestone for this ticket based on a cursory review.

Note: See TracTickets for help on using tickets.