manoharan-lab / holopy

Hologram processing and light scattering in python
GNU General Public License v3.0
131 stars 50 forks source link

Fixlenspolarizationrotation #373

Closed briandleahy closed 3 years ago

briandleahy commented 3 years ago

Fixes 371.

This fixes two bugs with the implementation of polarization rotation in the Lens theory. These issues were only present when the polarization was not along the x (or [1, 0]) direction. With this PR, the agreement between MieLens and Lens(Mie) does not change with polarization, and both theories produce rotationally-invariant results for rotationally-invariant objects (i.e. the parallel component of the field scattered from a sphere, evaluated at a position that is along the polarizazion vector, does not change as the polarization changes; this was always true for MieLens but was not true for Lens(Mie)).

Note that this does not change the calculations at all when the polarization is along the x-direction, so #350 is still open.

briandleahy commented 3 years ago

@ralex0 -- I made some slight edtis to the numexpr usage, but I don't have numexpr working on my machine. Can you check that the numexpr and no-numexpr pathwasy produce the same result? @barkls -- as we discussed on Thurs, I also threw in 2 minor fixes with this PR (faster test_interface and making the DDA tests pass). However I'm not sure if the DDA tolerances should be changed. If you want, we can leave the DDA tests as failing for this PR. Let me know.

barkls commented 3 years ago

Looks good, I just have a few additional comments:

1) It would be nice to have a short summary in this thread of what the bugs were for future reference, especially since the minor rewrite of the Lens class makes it hard to pinpoint from the code.

2) Please update the changelog.

briandleahy commented 3 years ago
  1. It would be nice to have a short summary in this thread of what the bugs were for future reference, especially since the minor rewrite of the Lens class makes it hard to pinpoint from the code.

There were two separate problems, both just involving polarization rotations.

The first problem was that the code rotated the object (by rotating the calculation of the scattering matrices) when the polarization was rotated. This was a simple bugfix.

The second problem was a more subtle error, corresponding to a math error in rotating the polarization (so apologies for having to read it in markdown). Take a look at Eq. 1 from the perspective paper below, which is correct for x-polarized light: Screenshot from 2020-10-26 11-38-36 Previously, the Lens theory was calculating the scattered fields by assuming that the component of the scattered field parallel to the polarization was the same, independent of whether the light was x-polarized or polarized in an arbitrary direction. This is equivalent to changing every "x-hat" in Eq. (1) into a "p-hat", where "p-hat" is the polarization direction (and "y-hat" with a vector perpendicular to the polarization). This is also incorrect, because the "x-hat"s appear for two separate reasons. The terms theta S x and phi S x capture the effects of the polarization of the incoming beam, and need to rotate when the polarization changes. But the terms (x cos(phi) + y sin(phi)) etc describe theta and phi unit vectors on the lens pupil, and do not rotate when the polarization changes. This is a conceptual difference, but it ends up being a small code change at the end -- the integrands just get a cos(phi - pol_angle) and sin(phi - pol_angle) instead of cos(phi) and sin(phi), lines 141-142 and 151-152.

If you want I can quickly TeX up the derivation and place it somewhere, perhaps here or on the group wiki.

pep8speaks commented 3 years ago

Hello @briandleahy! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 171:30: E201 whitespace after '['

Comment last updated at 2020-11-05 16:46:06 UTC
briandleahy commented 3 years ago
  1. Please update the changelog.

I've also added a line to the changelog, in 4c63307b

briandleahy commented 3 years ago

@barkls -- let me know if I missed anything or if this is ready to merge.

barkls commented 3 years ago

I think we're just waiting on a test of numexpr from @ralex0

briandleahy commented 3 years ago

@barkls -- I installed numexpr and checked this on my local machine. There were some issues, which I fixed. Those changes are now pushed (primarily in 7e63fc92 ). Please take a look and let me know if something seems off!

I did check that changing the numexpr expressions causes the tests to fail. I also adjusted the tests to compare the numexpr pathway against the numpy pathway, which in my opinion is what matters anyways.

barkls commented 3 years ago

Thanks for following up on this @briandleahy and patching things to work with numexpr. I suggested a context manager to handle the numexpr monkey-patches, but we can merge in as is if you don't feel like doing that. Just let me know what your preference is.

At some point it would be nice to get things like DDA and numexpr tested on Travis, but that's definitely beyond the scope of this PR.

briandleahy commented 3 years ago

Tests work with new commit. I'm ready to merge this in when you are.

barkls commented 3 years ago

good fix!