spacetelescope / poppy

Physical Optics Propagation in Python
https://poppy-optics.readthedocs.io
BSD 3-Clause "New" or "Revised" License
218 stars 72 forks source link

fix documentation of divergence angle #237

Closed mperrin closed 6 years ago

mperrin commented 6 years ago

Issue by douglase Tuesday Oct 03, 2017 at 20:54 GMT Originally opened as https://github.com/mperrin/poppy/pull/237


the returned divergence is the full angle not the half angle as previously implied. See definition in Kogelnik and Li, 1966, eq. 22, or the Newport Gaussian Beam Technical note (https://www.newport.com/n/gaussian-beam-optics)

Kogelnik, H., and T. Li. 1966. “Laser Beams and Resonators.” Applied Optics 5 (10): 1550–67. doi:10.1364/AO.5.001550.


douglase included the following code: https://github.com/mperrin/poppy/pull/237/commits

mperrin commented 6 years ago

Comment by mperrin Tuesday Oct 03, 2017 at 21:11 GMT


Huh, good catch. The other possible fix of course would be to remove the '2' from that line, so that it does return the half angle instead of full angle. You think it's better to just change the docs? Looking through a couple different references just now it doesn't seem like people are very consistent about whether they're talking about full angle or half angle... I'm fine with just fixing the docs to be consistent but wanted to at least ask the question.

mperrin commented 6 years ago

Comment by coveralls Tuesday Oct 03, 2017 at 21:14 GMT


Coverage Status

Coverage remained the same at 65.406% when pulling 40421ae4b8fe366dcee6468756e660c7c1ca6223 on douglase:patch-1 into 569d5c60f7cf86a256c661227bd6269de4144005 on mperrin:master.

mperrin commented 6 years ago

Comment by douglase Tuesday Oct 03, 2017 at 21:21 GMT


Kogelnik and Li drop the 2, as you suggest, and Newport and Verdeyen (Laser Electronics) define it as it is currently. It doesn't look like it's called anywhere else in POPPY, I was hesitant to break anyone's code in case they were using it appropriately but I would be inclined to drop the 2 if you don't think anyone else is using it (correctly).

mperrin commented 6 years ago

Comment by mperrin Tuesday Oct 03, 2017 at 21:32 GMT


It doesn't look like it's called anywhere else in POPPY, I was hesitant to break anyone's code in case they were using it appropriately but I would be inclined to drop the 2 if you don't think anyone else is using it (correctly).

In that case let me ping both @maciekgroch and @corcoted, who I know are using the Fresnel code. If either of you have got a spare moment would you be so kind as to take a look at this issue, and weigh in on which way you would rather see this inconsistency fixed? Thanks in advance!

mperrin commented 6 years ago

Comment by corcoted Wednesday Oct 04, 2017 at 13:57 GMT


This factor of two is inconsistent throughout the laser literature, too. As noted above, manufacturers tend to quote to full divergence angle and scientists tend to use the half angle. For example, the half angle is easier to use when doing paraxial propagation using the complex beam parameter equations, but the full angle is the number that the beam profiler reports.

I prefer the half angle myself, but I think either is acceptable if we're consistent and clearly document it.

mperrin commented 6 years ago

Comment by mperrin Tuesday Nov 14, 2017 at 01:45 GMT


@douglase in your last comment on this you said "I would be inclined to drop the 2 if you don't think anyone else is using it (correctly).", and I think we don't have any objections to doing it that way. Would you like to go ahead and modify the PR to fix is that way instead? Either way's fine with me, I just don't want to leave this an orphan hanging PR too long (like I did to your other one!)

mperrin commented 6 years ago

Comment by douglase Friday Nov 17, 2017 at 16:17 GMT


switched to half angle in 1c1ffca.

mperrin commented 6 years ago

Comment by coveralls Friday Nov 17, 2017 at 16:33 GMT


Coverage Status

Coverage increased (+0.03%) to 65.432% when pulling 1c1ffca2c698e1ab8d8713de7e278121c74919a4 on douglase:patch-1 into 569d5c60f7cf86a256c661227bd6269de4144005 on mperrin:master.

mperrin commented 6 years ago

Comment by mperrin Friday Nov 17, 2017 at 19:07 GMT


Thanks! I'm going to go ahead and merge this because all the actual code tests pass, apart from the known issue of the docs not building on Travis any more.