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: |
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
comment:2 Changed 18 months ago by
- Status changed from new to needs_info
comment:3 Changed 15 months ago by
- 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
- Cc gh-mjungmath added
comment:5 Changed 12 months ago by
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
- 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.
comment:7 Changed 12 months ago by
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: ↓ 9 Changed 12 months ago by
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
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
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: ↓ 12 Changed 12 months ago by
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: ↓ 16 Changed 12 months ago by
- 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 argumentscopy
andmutable
.
- 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 toTrue
for mutable inputsx
because (as discussed) this is not compatible with (1).
mutable=False
andcopy=False
MUST be an error for mutable inputx
.
We could add _test_...
methods that check this protocol.
comment:13 Changed 12 months ago by
- Cc nbruin added
comment:14 Changed 12 months ago by
- Cc dcoudert added
comment:15 Changed 12 months ago by
- Cc gh-mwageringel added
comment:16 in reply to: ↑ 12 Changed 12 months ago by
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()
andM(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 useM.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.
comment:17 Changed 11 months ago by
- Milestone changed from sage-9.2 to sage-9.3
comment:18 Changed 7 months ago by
- Cc pbruin added
comment:19 Changed 4 months ago by
- 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
- Milestone changed from sage-9.4 to sage-9.5
Setting a new milestone for this ticket based on a cursory review.
Some examples for comparison:
For basic python types, similar constructions return
list
,set
,dict
)tuple
)Regarding two examples of Sage objects,
Graph
s, a new object is returned, whether the old one was set immutable or notSet
s, which wrap around an immutable frozenset, the old object is returned