andykee / lentil

Heart-healthy physical optics
https://andykee.github.io/lentil/
Other
14 stars 6 forks source link

Consider deprecating in-place operations #43

Closed andykee closed 11 months ago

andykee commented 1 year ago

Proposed new feature or change:

Several of Lentil's primitive operations allow in-place execution, including Plane multiplication, Plane tilt fitting, Plane resampling/rescaling, and Wavefront propagation. The existence of these operations implies (at least in my mind) that their execution with inplace=True should be faster and/or use less memory. The reality is that with the exception of Plane.filt_tilt(), this assumption is not valid.

Beyond the lack of performance improvements, allowing in-place operations both complicates the library code and makes it less clear what is happening in user code.

Existing functions and methods can be arranged into the following two groups based on how they operate:

1. Methods that can never operate in-place Method Name
Plane.multiply
Plane.__imul__
Plane.resample
Plane.rescale
propagate_dft

These methods can never operate in-place because the nature of the operation requires copying or resizing arrays. For those methods, inplace=True is essentially just synctactic sugar for reassigning the new result to self.

2. Methods that modify underlying data and can be done in-place

Method Name
Plane.fit_tilt

These methods don't operate in-place by default, but could the option to specify inplace=True. The structure of the enclosed data remains intact, but it can be mutated in place.

Recommendation

The inplace parameter for all methods listed above should be deprecated. This will require modifying these functions and their supporting tests as well as updating documentation.

Backward compatibility

This is a breaking change. If this change is made in a pre-v1.0 release then no DeprecationWarning will be issued.