Avoid copy.deepcopy (restriction in FEniCS)

[ajafarihub]

A lot of models we practically wish to calibrate with probeye are developed in FEniCS, as it is the main simulation tool in our group. In FEniCS there is a restriction: we cannot deepcopy a FEniCS function, which is very often used in a simulator. Nevertheless, in probeye there are currently some places where the inference problem and its ingredients (mainly the forward model/simulator) are subjected to 'deepcopy'. This obviously raises failing errors as soon as there is any FEniCS function as attribute of the forward model we sub-class from probeye.ForwardModelBase. This (having FEniCS function as attribute of FW model) can certainly be avoided in simple examples of FW models, however, I find this as a major restriction in developing more involved FW models that we are going to sub-class from probeye.ForwardModelBase .

I am not sure which reasons are considered for making deepcopy of the inference problem in probeye. Maybe it has been a safe way of passing the same problem to several solvers. If so, perhaps a better approach instead of making deepcopy would be to introduce methods like 'reset_problem', such that the problem and particularly the forward simulator behind it can be returned to their original default state. Maybe this method can be considered as a must attribute of any forward simulator that a user is going to calibrate by probeye.

Edit:
Still a simple default method for such 'reset_problem' routine could be just making a deepcopy. The user can optionally provides/overwrites their own choice.

Edit-2:
There exist in total three of such copy.deepcopy operations in probeye:

probeye/probeye/definition/experiment.py

Line 40 in 41cf7ae

sensor_data_tuples = cp.deepcopy(sensor_data)

probeye/probeye/inference/solver.py

Line 34 in 41cf7ae

self.problem = cp.deepcopy(problem)

probeye/probeye/subroutines.py

Line 1295 in 41cf7ae

setattr(new_obj, attribute, deepcopy(getattr(ref_obj, attribute)))

The first one is OK, as it regards to the data, which is in standard python formats like array, float, int, etc. However, the other two cases raise errors.

[danielandresarcones]

After trying myself, I can confirm that, after eliminating the deepcopy occurrences, errors arise when the problem is passed consecutively to several solvers. As of now, I have no suggestion on the path of action for this.

[joergfunger]

Did you check, what exactly is internally changed when the problem is passed to a solver (resulting in problems when passing it to multiple solvers)?

[danielandresarcones]

I just checked it, the problem is in the function _translate_forward_models from SciPy's solver:

probeye/probeye/inference/scipy/solver.py

Lines 78 to 109 in 41cf7ae

	def _translate_forward_models(self):
	"""
	Translate the inverse problem's forward models as needed for this solver.
	"""
	logger.debug("Translate the problem's forward models")
	for fwd_name, fwd_obj in self.problem.forward_models.items():
	# create a full forward model from its hull where the sensors are not yet
	# connected to the experimental data
	forward_model_hull = self.problem.forward_models[fwd_name]
	forward_model = forward_model_hull.__class__.__bases__[0](
	fwd_name, *fwd_obj.args, **fwd_obj.kwargs
	)
	synchronize_objects(
	forward_model,
	forward_model_hull,
	exclude_startswith=("__", "_", "experiment_names"),
	)
	# add the experiments to the created forward model object by connecting
	# them with the respective sensors
	for exp_name in forward_model_hull.experiment_names:
	sensor_data = self.problem.experiments[exp_name].sensor_data
	forward_model.connect_experimental_data_to_sensors(
	exp_name, sensor_data
	)
	# this is a default preparation for increased efficiency
	forward_model.prepare_experimental_inputs_and_outputs()
	# finally, add the forward model to the problem
	self.problem.forward_models[fwd_name] = forward_model

The other solvers currently inherit from the SciPy's, therefore they behave in the same way.
When an inverse problem is initialized, the forward model is added as an instance of ForwardModelHull class that inherits from the derived ForwardModelClass (e.g. DerivedModelClass). When the previous function is called in the solver, a DerivedModelClass is instantiated and in the last line it substitutes the ForwardModelHull class.

probeye/probeye/inference/scipy/solver.py

Line 109 in 41cf7ae

self.problem.forward_models[fwd_name] = forward_model

This is possible because in lines 88-89 the function sees that the base class from the ForwardModelHull object is DerivedModelClass:

probeye/probeye/inference/scipy/solver.py

Lines 88 to 89 in 41cf7ae

	forward_model = forward_model_hull.__class__.__bases__[0](
	fwd_name, *fwd_obj.args, **fwd_obj.kwargs

However, once the function is called, the forward model remains as the instance of DerivedModelClass. Here it is where deepcopy is required. At the solver level, a deepcopy of the initial problem is created (i.e. the one with a ForwardModelHull instance as forward model), therefore the original problem is not modified when the previous functions are called. In that case, every solver starts with a ForwardModelHull instance as forward model. Without the deepcopy, all the solvers share the same original problem. Only the first one will see a ForwardModelHull instance as forward model, while the others will see a DerivedModelClass. When calling forward_model_hull.__class__.__bases__[0]() they will generate a ForwardModelBase instance instead of a DerivedModelClass one. This leads to NotImplementedError: No 'interface'-method defined for forward model.

A possible solution would be having different variables for the list of forward models and for their hulls in the solver level instead of overwriting the hull with the new model. I am not sure this is a good alternative, as it would mean to effectively copy the model in memory twice (plus one extra for each new solver), which can become problematic for large ones. Nevertheless, this would not prevent new issues to arise due to the manipulation of the problem.

[joergfunger]

Not sure of the proposed option would work, since even then you would need the deep copy to create the forward model. The reason for having the hull seems that we wanted to separate the problem definition from the actual implementation that have compute capabilities, e.g. to evaluate the prior using scipy or pyro (that is solver specific). However, the forward model is IMO not part of that and the translation of the experimental data (connect_experimental_data_to_sensors) and prepare_experimental_inputs_and_outputs might just be done once (so for the first solver if the forward model is not yet the derived class). I think the original problem was that some solver (pyro) needed different interfaces (tensors instead of numpy arrays), but since pyro has been removed, this is no longer required.
So one possible option could be to remove the deep copy and do the initialization of the forward model with the connection to experimental data just once.

[ajafarihub]

To my experience, rebuilding FW model should be OK, as the time needed for it is too small compared to solution phases. Removing deepcopy is also a simple work-around which has so far worked for me (with no error). Though, I would also prefer to be confident about this change. And to approve this change in the whole library, it looks that quite many of the tests should be revised (most likely by rebuilding FW models defined therein), because otherwise those tests would fail.

[joergfunger]

As I've mentioned, it is not sufficient to remove the deepcopy, but to perform the additional initialization only for the first conversion.

[aklawonn]

I just briefly looked at the problem, but I think the first two deepcopy appearances

probeye/probeye/definition/experiment.py

Line 40 in 41cf7ae

sensor_data_tuples = cp.deepcopy(sensor_data)

and also

probeye/probeye/inference/solver.py

Line 34 in 41cf7ae

self.problem = cp.deepcopy(problem)

should not result in errors. In the first instance, deepcopy only applies on data as Abbas already pointed out. But also in the second appearance the deepcopy should only apply on problem-objects that only have the forward model hulls, and not the full forward models.

As I see it, the problem is triggered by the third deepcopy appearance

probeye/probeye/subroutines.py

Line 1295 in 41cf7ae

setattr(new_obj, attribute, deepcopy(getattr(ref_obj, attribute)))

and its use in the _translate_forward_models method. Maybe try to replace that third deepcopy appearance by a simple copy and keep the other two deepcopys. All the tests pass when this is done.