Refactoring SHWFS code #369
Conversation
1. Refactored code in sub_sampled_optics.py by removing extra white spaces 2. Comparisons with None values using is/is not instead of =/!= 3. Solve minor bug in 'get_centroids' method due to wrong code indention
Codecov Report
@@ Coverage Diff @@
## develop #369 +/- ##
===========================================
- Coverage 71.02% 71.00% -0.02%
===========================================
Files 17 17
Lines 5732 5733 +1
===========================================
Hits 4071 4071
- Misses 1661 1662 +1
Continue to review full report at Codecov.
|
|
@fanpeng-kong Thank you for this pull request! Much appreciated. We will take a look as soon as time permits. |
|
FYI @remorgan123, would you have time to take a look at this PR with some updates to the SHWFS code, please? |
| else: | ||
| intensity_array = sub_wf.intensity | ||
| self.centroid_list[:,i,j] = cent_function(intensity_array,**kwargs, relativeto = relativeto) |
There was a problem hiding this comment.
I'm not sure this is a bug since the asFITS case wouldn't lead to a centroid list if the function is only called inside the else statement - @remorgan123 do we have a test that includes this line?
There was a problem hiding this comment.
@douglase it looks like there is no unit test coverage at all for any of the sub sampled optics here - See the codecov report:
https://codecov.io/gh/spacetelescope/poppy/compare/15a427893e59d22bb70172c660515edb6cebffd7...faa0187b70f4fa02a2bd783a54bc052ce6e43118/src/poppy/sub_sampled_optics.py?before=poppy/sub_sampled_optics.py
There was a problem hiding this comment.
him, not sure linking into codecov works well... if you go to the codecov link at the bottom of this PR, click to files, and browse to this file, then it shows 0% coverage.
There was a problem hiding this comment.
Sorry, I overlooked the asFITS case. I encountered the following error earlier while running Shack Hartmann Wavefront Sensor Demo.ipynb by changing the pixel_pitch from 2.2um to 5um. This leads to negative indices of XLOC and YLOC which I am not sure if it's intended by fwcentroid:
~/Projects/Python/Optics/poppy_sim/shwfs_demo.py in
88 shwfs.sample_wf(wf_flat)
89 shwfs.get_psfs()
---> 90 flat_centroid_list = shwfs.get_centroids()
91
92 #sample actual wf and propagate to detector:
~/Projects/Python/Modules/poppy/poppy/sub_sampled_optics.py in get_centroids(self, cent_function, relativeto, asFITS, **kwargs)
272 else:
273 intensity_array = sub_wf.intensity
--> 274 self.centroid_list[:, i, j] = cent_function(intensity_array, **kwargs, relativeto=relativeto)
275
276 self._centroided_flag = True
~/Projects/Python/Modules/poppy/poppy/utils.py in measure_centroid(HDUlist_or_filename, ext, slice, boxsize, verbose, units, relativeto, **kwargs)
1024 image = image[slice, :, :]
1025
-> 1026 cent_of_mass = fwcentroid(image, halfwidth=boxsize, **kwargs)
1027
1028 if verbose:
~/Projects/Python/Modules/poppy/poppy/fwcentroid.py in fwcentroid(image, checkbox, maxiterations, threshold, halfwidth, verbose)
102 YLOC = j
103 # print " (%d, %d)" % (XLOC, YLOC)
--> 104 SUM += image[j, i]
105 XSUM += XLOC * image[j, i]
106 XSUM2 += XLOC ** 2 * image[j, i]
IndexError: index 30 is out of bounds for axis 0 with size 30
My previous wrong intent bypassed the asFITS case and therefore hided the error but also led to no centroid estimation.
The error itself may not necessarily be a bug but could depend on a correct selections of the pixels per lenslet and halfwidth:
Line 29 in 15a4278
@mperrin Could you please point me to some written materials on the two JWST materials so I can investigate further?
Line 20 in 15a4278
There was a problem hiding this comment.
yes, common error in my experience with fwcentroid() that usually requires adjusting the halfwidth size.
There was a problem hiding this comment.
Yes I agree the fwcentroid function isn't so sophisticated as it could be (by design... the flight version of this algorithm runs in a very tiny and not so smart computer.) We could make the version here more intelligent about choosing good default parameter sizes, or at least giving a more informative error message that says the box size should be increased.
|
Thanks @fanpeng-kong for this contribution! Much appreciated. Looks good and I will go ahead and merge this now. However as a broader question to you, @douglase, and @remorgan123: As I noted in my comment the other day, right now there's no test coverage right now at all for any of the SHWFS code. It would be nice if we could improve that at some point. I think some of the code from the example notebook could be adapted to make a test case fairly easily for this. @douglase let me know if you agree that could maybe be a good student project sometime? Not immediately urgent but would be nice to have at some point at least basic test coverage on this code. Cheers and have a good weekend, all. |
spaces
indention