API update: rename Zernike functions (no change in functionality, just name update, with back compatibility) #471
Conversation
|
FYI @grbrady @obi-wan76 @ivalaginja @douglase @jlumbres @sdwill on a proposed small change in the poppy API. Any comments welcome. |
Codecov Report
@@ Coverage Diff @@
## develop #471 +/- ##
===========================================
+ Coverage 74.72% 74.73% +0.01%
===========================================
Files 18 18
Lines 6472 6476 +4
===========================================
+ Hits 4836 4840 +4
Misses 1636 1636
Continue to review full report at Codecov.
|
| @@ -211,7 +211,7 @@ def test_opd_expand(npix=512, input_coefficients=(0.1, 0.2, 0.3, 0.4, 0.5)): | |||
| basis[idx] *= coeff | |||
|
|
|||
| opd = basis.sum(axis=0) | |||
| recovered_coeffs = zernike.opd_expand(opd, nterms=len(input_coefficients)) | |||
| recovered_coeffs = zernike.decompose_opd(opd, nterms=len(input_coefficients)) | |||
There was a problem hiding this comment.
I'd probably change the name of the test function from test_opd_expand() to test_decompose_opd() (a few lines up) for continuity
| just the first 8 for JWST. This is because focus and spherical are not orthogonal | ||
| when in hexike versions on the JWST aperture, so "spherical was booted out of the picture" | ||
|
|
||
|
|
||
|
|
||
| Parameters | ||
| ---------- |
There was a problem hiding this comment.
Further down on line 831 (the class docstring for Segment_PTT_Basis), the line should be changed from "the opd_from_zernikes or opd_expand_segments functions." to "the compose_opd_from_basis or decompose_opd_segments functions."
| def opd_expand(opd, aperture=None, nterms=15, basis=zernike_basis, | ||
| **kwargs): | ||
| def decompose_opd(opd, aperture=None, nterms=15, basis=zernike_basis, | ||
| **kwargs): | ||
| """Given a wavefront OPD map, return the list of coefficients in a |
There was a problem hiding this comment.
The decompose_opd function's docstring (line 951 and 952) references opd_expand_nonorthonormal and opd_expand_segments when it should now reference "ecompose_opd_nonorthonormal_basis and decompose_opd_segments
Also, further down in the docstring on line 982, it says opd_expand when it should now say decompose_opd
poppy/zernike.py
Outdated
| def opd_expand_nonorthonormal(opd, aperture=None, nterms=15, basis=zernike_basis_faster, | ||
| iterations=5, verbose=False, **kwargs): | ||
| def decompose_opd_nonorthonormal_basis(opd, aperture=None, nterms=15, basis=zernike_basis_faster, | ||
| iterations=5, verbose=False, **kwargs): | ||
| """ Modified version of opd_expand, for cases where the basis function is |
There was a problem hiding this comment.
Docstring says opd_expand when it should say decompose_opd
| def opd_expand_segments(opd, aperture=None, nterms=15, basis=None, | ||
| iterations=2, verbose=False, ignore_border=None, **kwargs): | ||
| def decompose_opd_segments(opd, aperture=None, nterms=15, basis=None, | ||
| iterations=2, verbose=False, ignore_border=None, **kwargs): | ||
| """ | ||
| Expand OPD into a basis defined by segments, typically with piston, tip, & tilt of each. |
There was a problem hiding this comment.
On line 1178, it says opd_expand_nonorthonormal when it should now say decompose_opd_nonorthonormal_basis
| def opd_from_zernikes(coeffs, basis=zernike_basis_faster, aperture=None, outside=np.nan, | ||
| **kwargs): | ||
| def compose_opd_from_basis(coeffs, basis=zernike_basis_faster, aperture=None, outside=np.nan, | ||
| **kwargs): |
There was a problem hiding this comment.
The example in the docstring for compose_opd_from_basis still calls the old function name
poppy/tests/test_zernike.py
Outdated
| @@ -222,16 +222,16 @@ def test_opd_expand(npix=512, input_coefficients=(0.1, 0.2, 0.3, 0.4, 0.5)): | |||
| # We do the test in this same function for efficiency | |||
|
|
|||
|
|
|||
| recovered_coeffs_v2 = zernike.opd_expand_nonorthonormal(opd, nterms=len(input_coefficients)) | |||
| recovered_coeffs_v2 = zernike.decompose_opd_nonorthonormal_basis(opd, nterms=len(input_coefficients)) | |||
| max_diff_v2 = np.max(np.abs(np.asarray(input_coefficients) - np.asarray(recovered_coeffs_v2))) | |||
| assert max_diff_v2 < 1e-3, "recovered coefficients from wf_expand more than 0.1% off" | |||
|
|
|||
|
|
|||
| def test_opd_from_zernikes(): | |||
There was a problem hiding this comment.
I'd change this test function name to test_compose_opd_from_basis
|
Oh, these functions also get called in webbpsf, so we may want to update webbpsf to call these new functions (they're called in optics.py and opds.py. It doesn't have to be immediate since they are backward compatible, but it should definitely happen at some point. |
|
Thank you @shanosborne, good catches all. I must not have had the "check doc strings" option checked in pycharm when I did the refactoring; whoops. Much appreciated! All updated. Agreed re: webbpsf updates similarly; it makes sense to do that shortly, probably right after we get poppy 1.0 released so we can depend on that version number in webbpsf. |
A 1.0 release is a good time to update the API, so I want to take the opportunity to improve slightly a part of the API that's bugged me for a while. This PR introduces new names for the various Zernike composition/decomposition functions, for better consistency with typical usage.
opd_expand->decompose_opd. This is consistent with typical usage (and the vocabulary used with the JWST WSS for instance) about phase decomposition.opd_from_zernikes->compose_opd_from_basis. This name makes it clearer this is an inverse function to the decomposition, and also it fixes the not-ideal situation of having "zernikes" baked into the function name when instead that function can be used with hexikes or any other arbitrary basis, not just Zernikes.The existing names are retained as back-compatible aliases, so no changes are needed in existing code. Both the old and new names will work interchangeably.