Redundancy of Function.copy(deepcopy=False)

[Jan Blechta]

As pointed out by @pefarrell, it is not clear whether there are use cases for Function.copy(deepcopy=False) over Python assignment. At least it causes some confusion.

Possibly Function.copy(deepcopy=False) could be removed and Function.copy() would default to deep copy. Optimally it would be the case also on the C++ side.

If the decision will be to keep it, then improvement of its documentation would be desirable.

The snippet below demonstrates the behaviour of the two.

from dolfin import *
mesh = UnitSquareMesh(10, 10)
V = FunctionSpace(mesh, "CG", 1)
u = Function(V)

v = u.copy(deepcopy=False)
print u.id(), u.vector().id(), v.id(), v.vector().id()

v = u
print u.id(), u.vector().id(), v.id(), v.vector().id()

results in

4 6 14 6
4 6 4 6

[Martin Sandve Alnæs]

Agree very much.

[Jan Blechta]

What would the deprecation mechanism?

>>> u.copy(deepcopy=True)

>>> u.copy(deepcopy=False)
***Warning: deprecated, will be removed

>>> u.copy() # now shallow copy
***Warning: will change to deepcopy next (or another?) release!

[Martin Sandve Alnæs]

Looks good. Given that shallow copy doesn't seem to make any sense or seems dangerous, I wouldn't wait more than one release. Maybe even define it as a bug and just fix it? Is there any valid use case for the shallow copy that doesn't lead to trouble?

[Jan Blechta]

It is certainly part of someone's workflow:

[Martin Sandve Alnæs]

:)

[Patrick Farrell]

Why is shallow copying 'dangerous'? It's a perfectly reasonable thing to do, I think.

[Jan Blechta]

You probably mean v = u. If not, could you point us to the example where v = u.copy(deepcopy=False) is useful? Remember, for the latter it holds v.id() != u.id() and v.vector().id() == u.vector().id().

[Martin Sandve Alnæs]

Yes, for clarity: v = u is not a shallow copy, it copies a reference to the same object.

Shallow copy would be v = u.copy(deepcopy=False), which results in modifications to v.vector() reflected back on u because u.vector() is the same object.

[Martin Sandve Alnæs]

I can see possible uses of shallow copy for optimization for users who really understand what they're doing. But the default should at least be deep copy, to avoid this confusing behaviour:

from dolfin import *
mesh = UnitSquareMesh(10, 10)
V = FunctionSpace(mesh, "CG", 1)
u = Function(V)
v = u.copy()
v.vector()[:] = 4.0
u.vector()[:] = 3.0 # **
assert u.vector().norm('linf') == 3.0
assert v.vector().norm('linf') == 4.0  # fails because v is now 4, as set on line **

[Jan Blechta]

I can see possible uses of shallow copy

Maybe something like:

u = Function(W)
v = Function(W)
F1 = u*v*dx

u = Function(W)
v = u.copy(deepcopy=False)
F2 = u*v*dx # same code generated as for F1

(not tested)

Anyway, I'm quite confused, @pefarrell, that first you claim that deepcopy=False is useless and now you object removing it.

[Patrick Farrell]

I'm sorry that my comment was confusing. Please disregard it, what I actually meant isn't important. I have no objection whatsoever to removing deepcopy=False.

[Jan Blechta]

@martinal, the implementation shows that it is easily replaceable if we remove it.

Possible argument for default deepcopy=False is consistency with Function.sub(i, deepcopy=False) and Function.split(deepcopy=False). (The latter might be subject to some changes when @garth-wells fixes #194.) I don't think that we need to follow this argument.

[Prof Garth Wells]

For sub functions, from C++ we could have: Function.sub(i).copy().

[Prof Garth Wells]

@blechta Did you you make some recent progress on this?

[Jan Blechta]

Work in progress. Got into trouble with this construct

function.sub(0, deepcopy=True).vector()

This code is in demos to compute l^2 norms of subfunctions (for some unknown reason, just to show some numbers I guess).

My view is that function.sub(0, deepcopy=True) is a function living on a collapsed space which was just willy nilly constructed behind the scene and is destroyed when the garbage is collected, possibly on a next line. I would prefer to remove that functionality.

Writing this summary and thinking of #918, I am quite sure I will suggest removing the functionality. Expect a PR.