lightpipes icon indicating copy to clipboard operation
lightpipes copied to clipboard

Inconsistent definitions of waist w and xshift

Open jmmelko opened this issue 4 years ago • 2 comments

I love Lightpipes but there are still a few inconsistencies in parameters definitions, some of them going beyond mere cosmetical problems.

  • GaussBeam requires w0, the beam radius, while for PlaneWave w is the wave diameter. I understand the absence of subscript 0, but w should always be the radius, not the diameter.
  • GaussBeam accepts xshift, yshift, while its documentation states it accepts x_shift. x_shift (with underscore) seems to be the definition used in other functions. Assigning a value to x_shift with GaussBeam throws the error GaussBeam() 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

jmmelko avatar Feb 02 '21 13:02 jmmelko

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.

ldoyle avatar Feb 08 '21 23:02 ldoyle

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.

jmmelko avatar Feb 09 '21 06:02 jmmelko