Fix arbitrary_basis clipping aperture

[mperrin]

Issue by kvangorkom
Wednesday May 17, 2017 at 18:33 GMT
Originally opened as mperrin/poppy#222

---

Addresses #221.

The implementation here varies slightly from what I did over in #221. Rather than pad the aperture array, I'm calculating what the new array size should be, passing this to zernike_basis as npix, and then cutting the circular basis down before the aperture is cut out of it and the basis re-orthonormalized.

To handle the optional rho and theta arguments, I'm padding these arrays with nonsense values. I think this shouldn't have any negative consequences, but I am a bit uncomfortable with handling them this way.

(Also adds a check that the aperture is passed in as a square array.)

---

kvangorkom included the following code: https://github.com/mperrin/poppy/pull/222/commits

[mperrin]

Comment by coveralls
Wednesday May 17, 2017 at 18:50 GMT

---

Coverage decreased (-0.2%) to 64.717% when pulling 1cb191d on kvangorkom:arbitrary_basis into 5a45a28 on mperrin:master.

[mperrin]

Comment by mperrin
Wednesday May 17, 2017 at 19:13 GMT

---

Overall looks good. I made a couple comments in-line on the code.

If you're feeling ambitious, would you be willing to put together a unit test for this function that demonstrates it works as desired? The coveralls check reminded me that we don't have any test code for arbitrary_basis at all yet...

[mperrin]

Comment by coveralls
Tuesday May 23, 2017 at 17:17 GMT

---

Coverage increased (+0.5%) to 65.439% when pulling e071841 on kvangorkom:arbitrary_basis into 5a45a28 on mperrin:master.

[mperrin]

Comment by kvangorkom
Tuesday May 23, 2017 at 17:31 GMT

---

The latest commits should hopefully address all your comments!

For unit tests, I just duplicated the tests that were being performed for the other bases--one check for the normalization and one for orthogonality.

A few things I noticed that probably deserve their own issues :

1. I'm not sure that test_zernikes_rms is performing the check quite correctly. The line

assert 1.0 - rms < 0.001, "Zernike(j={}) has RMS value of {}".format(j, rms)

ought to be

assert abs(1.0 - rms) < 0.001, "Zernike(j={}) has RMS value of {}".format(j, rms)

correct? Otherwise, RMS values > 1 sneak on by. (The latter is how I formulated it in test_arbitrary_basis_rms)

2. I'm not sure the different combinations of rho, theta being None are handled correctly (theta=None with rho defined hits an array broadcasting error, rho=None with theta defined runs successfully with unexpected results). I get the same behaviors with zernike_basis as with arbitrary_basis, so I left it alone for now.
3. hexike_basis ignores the outside argument.

[mperrin]

Comment by mperrin
Tuesday May 23, 2017 at 18:37 GMT

---

Nice work. For your suggested other issues those all look like good catches, thanks very much. I'll spawn a new issue and fix those myself.

[mperrin]

Comment by mperrin
Tuesday May 23, 2017 at 18:56 GMT

---

This all looks good, so I'm going to go ahead and merge. Thanks much @kvangorkom !