search

mperrin / poppy

Physical Optics Propagation in Python
BSD 3-Clause "New" or "Revised" License
175 stars 41 forks source link

Fix misc issues from #223 #224

Closed mperrin closed 7 years ago

mperrin commented 7 years ago

@kvangorkom Take a look and see if you think this addresses the points you raised.

(Ironically, while adding the outside option to the hexike function, at one point I had a bug such that hexike_basis always returned all NaN for all pixels always. And the whole test suite passed anyway! Because all the comparisons were only checking the valid non-Nan pixels. Ha. So I added a new test case for that too. )

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 65.439% when pulling 3218dadc58aeb8e44607a14e85c35c34c18f3c43 on fix_223 into 6b01e66b685cf2c82e89805c28ac502287a034fb on master.

kvangorkom commented 7 years ago

Hm. poppy.zernike.zernike_basis(theta=theta,rho=None) is still sneaking by. It looks like zernike_basis checks only for rho, and, if it doesn't see it, silently tosses out the theta argument and circumvents the check you added in zernike. (So it essentially behaves as if you didn't pass in either one.)

The outside option doesn't appear to be quite right—the Gram-Schmidt process clobbers any value adopted before the re-orthonormalization. In arbitrary_basis, I simply added an extra line at the end to do this (as well as cast the list of terms to an ndarray for consistency with zernike_basis).

mperrin commented 7 years ago

After much delay I went back and implemented the fixes you suggested here, @kvangorkom. Thanks for the helpful close reading of my code! Shouldn't have taken this long to fix, but I hadn't had time to work on webbpsf or poppy at all for the last two months until today (when it became critical suddenly since I had to fix breakage from the astropy 2.0 release).

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.002%) to 65.383% when pulling ecef74e722b51de034f0cc16fc2a7e37ed4dd3c7 on fix_223 into 5c2b8b47ef73e0817b58461940bc1cc44077ad51 on master.