Open jmmelko opened 3 years ago
Dear jmmelko,
thanks for your feedback.
Regarding the GaussBeam
, you are right, it seems x_shift
/y_shift
are the better choice. Maybe Fred or I can update this in the coming week or so (feel free to change yourself and create a Pull Request, best try to keep it backward compatible by optionally probing for xshift
/yshift
in the kwargs
)
@FredvanGoor regarding the w
of the plane wave, I agree D
/d
or R
/r
would be more instructive. Since most other functions are defined via some radius, this would be better here, too. However, that would change the behaviour of existing code. It seems the function is fairly recent (introduced in 2.0), so do you think we can risk the change?
Regarding the Field
, your other Git Issue is absolutely right it should be documented better, however I'm not sure about adding pythonic gimmicks to the field. It acts more like a container for the parameters grid size, lambda,... and the actual complex field data (Field.field
which is a numpy array having all the nice capabilities like .size
,.shape
and so on). If documented better, this seems sufficient to me.
Ok, I’ll do that.
I fail to grasp what would be the risk of changing the PlaneWave parameter: backward compatibility or misuse? It is already confusing as it is.
I love Lightpipes but there are still a few inconsistencies in parameters definitions, some of them going beyond mere cosmetical problems.
w0
, the beam radius, while for PlaneWavew
is the wave diameter. I understand the absence of subscript 0, butw
should always be the radius, not the diameter.xshift
,yshift
, while its documentation states it acceptsx_shift
.x_shift
(with underscore) seems to be the definition used in other functions. Assigning a value tox_shift
with GaussBeam throws the errorGaussBeam() got an unexpected keyword argument 'x_shift'
Besides, I may we wrong, but to be more Pythonic I would suggest to override the len() function for Field objects so that len() returns
.siz