Refactor the single mode fiber injection unit #192
Conversation
Also removes a deprecated, outdated function.
ehpor
force-pushed
the
feature/single_mode_fiber_injection_refactor
branch
from
45435bd
to
2b6e32a
Compare
Codecov Report
@@ Coverage Diff @@
## master #192 +/- ##
==========================================
+ Coverage 81.40% 81.56% +0.15%
==========================================
Files 97 97
Lines 7266 7334 +68
==========================================
+ Hits 5915 5982 +67
- Misses 1351 1352 +1
... and 2 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
@syhaffert This wasn't ready for review. Now it is. Both were fixed already. |
Just because I saw it.
hcipy/optics/fiber.py
Outdated
|
|
||
| # Compute and normalize the Gaussian function. | ||
| res = np.exp(-r2 / ((0.5 * mode_field_diameter)**2)) | ||
| res /= np.sum(np.abs(res)**2 * grid.weights) |
There was a problem hiding this comment.
We know theoretically what the normalization factor should be, right? It's 1/sqrt(2*pi * sigma^2) with some correction for the pixel size. Wouldn't that be a better normalization? I ran into issues before where I couldn't rely on numerical normalization due to a finite grid size.
There was a problem hiding this comment.
Fixed. There is no correction for pixel size. However, there is a change inside the exponential itself, since we are normalizing for the square, not the Gaussian itself.
BTW, should we then also remove the normalization inside the SingleModeFiberInjection element itself? That feels wrong though.
There was a problem hiding this comment.
Yeah I think we need to leave the normalization inside that class just in case some weird mode is used there.
There was a problem hiding this comment.
The normalisation is important, otherwise, the throughput will look oddly too high. Also, the mode_field_diameter requires a clear definition. It can change in textbooks. Usually, I use the same as Thorlabs: 1/e2 on the power (https://www.thorlabs.com/NewGroupPage9_PF.cfm?ObjectGroup_ID=949). So it would be:
res = np.exp(-r2 / ((mode_field_diameter)**2))
res *= np.sqrt(2/np.pi)/mode_field_diameter
(from heart, but please double check!)
There was a problem hiding this comment.
I kept the numerical normalization inside the SingleModeFiberInjection class.
@sylacour Thanks. I indeed had a wrong definition of the MFD. (I intended to use the 1/e2 one, but I made a mistake in evaluating the Gaussian.) Your from-heart formula was actually correct, except for exchanging MFD with the mode field radius. I also added a test to make sure that the 1/e definition is always checked for this function. The normalization was already tested.
|
@syhaffert Good catch. That bug has been there for a while then. I put the conjugate on the input field. I added an imaginary mode in the test as well. The test doesn't pass without the conjugation. |
There was a problem hiding this comment.
I found another missing conjugate.
Yeah I think it has been here for a while. That just shows that this part of the code is not used that much. Do we need to add a np.real to the outcome of the dot product? Intensity is by definition real and adding that np.real will make sure it is compliant.
edit: That was a stupid comment from me. The dot product is the complex amplitude of the mode. no need to force realness here!
This correctly deals with fiber modes with a nonzero imaginary component.
The square should have been taken over the absolute value.
Converts the single-mode fiber detector into a single-mode fiber injection optical element.
__init__.py.Fixes #191.