Refactor light-leak function and remove light-leak files from repository #180
Conversation
There was a problem hiding this comment.
As written the change from rebin to the resample method are not equivalent. I have to say that I don't understand the inclusion of the factor of 0.25, though it does conserve the sum of the light leak image pixels. I found that rebin and resample don't lead to identical results and both differ a bit from the results using IDL rebin. Also I discovered that the tests we run don't test the rebinning because all of the test files are 1024 x 1024. In fact 2048 x 2048 images are quite rare in the archive.
… resolution, i.e. 2048 x 2048
…lated light leak subtracted image to test rebinning code. Also added test that uses these files.
|
After adding those two full-resolution light-leak files, I'll point out that this PR actually makes the repo even larger than it already was. If those are going to be used as test data, I would suggest they be stored remotely and we can pull them down using the same strategy that is employed here. |
|
By the repo, do you mean the package to be released? I thought we decided that none of the tests or test data would be included in that. If you just mean the github repo, I didn't think there was a size problem for that. |
I was forgetting that this is test data that can be dropped from the package that can be pushed to PyPI. Yes, that is true, though I would still be reluctant to continue to add large files to the git repository even if they do not get included in the package. Could we instead just resample one of the existing 1024-by-1024 images to 2048-by-2048 on the fly before testing? |
|
Re: the factor of 0.25, what is the actual intention in calling rebin in the original code? As written now (with the factor of 0.25), the interpolated stray light map will have 1/4 the intensity of the original stray light map as the If I understand the actual intention of this scaling factor, an even more correct solution would be to reproject the stray light map to a WCS with the appropriate size and resolution while conserving flux. |
|
So the scaling factor comes from the fact that the lightleak images are summed by a factor of 2 on chip. So the image pixels are a factor of 2 (in each dimension) larger, physically than a full resolution image. To simulate a full resolution image then we want to split the flux in each pixel into four pixels. So a flux conserving approach would be better, though I don't think any reprojection would be needed. In practice I think resample is adequate given the accuracy needed. There are a variety of approximations involved in doing a simple subtraction as is done by the original IDL code. |
| they need not be the same multiple | ||
| if in_map.dimensions != leak_map.dimensions: | ||
| leak_map = leak_map.resample(u.Quantity(in_map.dimensions)) | ||
| leak_map *= 0.25 |
There was a problem hiding this comment.
Should we add a comment here about why this is multiplied by 0.25?
There was a problem hiding this comment.
Yes, will do. Also need to restrict the condition to cases in which the in_map.dimensions are twice the leak_map.dimensions.
I was also thinking that we should probably issue a warning if the image has the wrong dimensions generally. The use of the routine as written is supposed to be for composite images that are either 1024 x 1024 or 2048 x 2048. It won't work for images with different sizes.
|
Should we add a changelog entry for this pull request too? |
Changed error for input file with light leak previously removed to a warning. Made the dimensions test more specific for case of 2048x2048 input image. Added comment.
|
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
This PR removes the light-leak files from the repository in favor of pulling them from one of two possible solarsoft mirrors with the goal of reducing the total size of the repository. It also significantly refactors the light-leak removal function.
At a high-level, the changes to the light-leak function are:
sunpy.map.Mapobject, not just a bare arrayrebinfunction has been removed in favor of just callingMap.resampleto the resolution of the input map. My understanding is that this is basically what was being achieved by rebinning from 1024 to 2048 and then dividing by 4.leak_imageis now calledleak_mapto reflect the fact that it is asunpy.map.Map, not just a bare arrayk_factis now calledscalebecause I foundk_factto be ambiguous. The namescalereflects the fact that the light-leak image is being scaled by this factor.