RiemannianMetric.copy not deep-copying parameters

[ddundo]

L190 in metric.py metric.set_parameters(self.metric_parameters.copy()) doesn't deep copy the metric_parameters dictionary.

MFE:

from firedrake import *
from animate.metric import RiemannianMetric

mesh = UnitSquareMesh(3, 3)
P1_ten = TensorFunctionSpace(mesh, "CG", 1)
metric = RiemannianMetric(P1_ten)

x, y = SpatialCoordinate(mesh)
h_min = Function(FunctionSpace(mesh, "CG", 1)).interpolate(x / 2.)
mp = {"h_min": h_min, "h_max": 1.0}
metric.set_parameters(mp)

metric_copy = metric.copy(deepcopy=True)
mp_copy = metric_copy.metric_parameters

print(id(mp) == id(mp_copy))                    # False
print(id(mp["h_min"]) == id(mp_copy["h_min"]))  # True
print(id(mp["h_max"]) == id(mp_copy["h_max"]))  # True

[stephankramer]

Don't want be contrarian, but I don't really think this is a bug - and tbh "fixing" it would be more surprising than the current situtation. The copy of a metric already has an independent parameters dictionary, it just gets set with the same contents as the original metric. If you want to change its values, where value might be a function, you overwrite it, including replacing it with a different function. If I have a metric parameters dictionary with a minimum edge function, and I use it to create a metric, and then make a deepcopy of it, or multiple - and I then change the value in the minimum edge function, I expect all copies to be applying that same changed minimum edge length. Any other way would just not be python-esque I think. In my opinion the deepcopy=True refers to what a Function stores as a container, which is the function values, not anything else that's used to define it, e.g. I wouldn't expect it to make an independent copy of the mesh either. Also if you do start make copies of the value in the dictionary, where do you stop? What happens if I put a metric in the parameters dictionary?

Apart from that in your MFE, I also don't see what you're expecting in the case of h_max. There is no such thing as making an independent copy of a float - floats are immutable objects and whether or not a float with the same value is a different object or not, is an implementation issue (for small integers for instance the same value integer object will be reused even when "generated" in separate contexts).

[ddundo]

Thanks @stephankramer, that makes sense! I was worried about this in particular when I was deep-copying a metric with variable metric parameters, where I specifically wanted to make sure that they are distinct for each metric. And you're right that just calling mp_copy.set_parameters(new_mp) does that! I don't remember now what I was doing exactly when I came across this issue but surely something wrong...

@jwallwork23 just tagging you here so you don't miss it - I'm happy to leave this as it is :)

[jwallwork23]

Makes sense, thank you @stephankramer. I will revert the corresponding changes in #99, although keep that PR to introduce the parameter docs and do some refactoring. I gather this issue should be closed but feel free to reopen it if there's something I missed.