Combine images

[bensutlieff]

Describe your changes

Tasks related to issue Combine images #236 :

• Added additional tests to test_combine.py to confirm that combine.py behaves as expected
• Verified the error propagation calculation in combine.py
• Fixed bugs and typos in combine.py
• Fixed a couple of typos in readme.md

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)

Reference any relevant issues (don't forget the #)

Combine images #236

Checklist before requesting a review

• I have linted my code
• I have verified that all unit tests pass in a clean environment and added new unit tests, as needed
• I have verified that all docstrings are properly formatted and added new documentation, as needed

[kjl0025]

These are all supposed to be "identical" frames, right? My suggestion would be to implement the methodology for computing the combined err and combined dq from corgidrp.darks.build_trad_dark(), which uses corgidrp.darks.mean_combine(). build_trad_dark() in lines 215-234 masks pixels that have a non-zero DQ value in the computation of the error, and if a pixel has more than half of its values flagged in the stack, the combined DQ value for that pixel is flagged as unreliable (with a value of 256) [or we could mark a DQ value as 1 if no frames in the stack for that pixel are good]. The error includes "systematic error" (coming from the err from each frame in the stack) as well as the statistical error (coming from the variation from frame to frame). Combining the "systematic" error with the statistical error in this way assumes corgidrp.l2a_to_l2b.add_photon_noise() has not been applied to the frames' err since that adds in essentially an estimate of the statistical error per frame, whereas computing it as in build_trad_dark() computes that statistical error over the stack directly. A discussion on that point about add_photon_noise() can be found here: #124

[semaphoreP]

@kjl0025 this step function will be run at L3->L4 processing, so after photon noise has been propagated into the error array. I worry that the method you suggested will double count photon noise. I think we can assume all the sources of err have already been propagated into the error array, and we don't need to look at the variation from frame to frame.

[kjl0025]

Yes, issue #124 refers to our previous discussion about assigning a more accurate statistical noise compared to what is assigned per frame in add_photon_noise() once a dataset is combined into a single frame. One option is to leave add_photon_noise() out of a recipe if the intended end product will be a combined frame. In the rare case that only 1 frame is taken for a dataset, add_photon_noise()` would be a useful estimate, though.

[semaphoreP]

I'm not sure why we would leave add_photon_noise() out. My thinking is that the err array should be an accurate representation of the noise. If we need to account for other noise terms (e.g., by taking the stddev from a stack), we should do that in a separate step function and add it to the existing err array. By the time we're at L3 data products, I want all sources of uncertainty from detector noise sources in the err array already, and not have to further compute additional error terms from the data.

[kjl0025]

Yeah, you can see the discussion in that issue for proposed ways around that (like taking the standard error of the stack and dividing it up among the frames in the stack). Perhaps to address that requires an edit of add_photon_noise(). The other points, though, are relevant to this PR.

[semaphoreP]

Looks good to me!

[semaphoreP]

@kjl0025 I looked through that discussion, and I'm not sure if there's anything in there that's missing in this PR.

[kjl0025]

Oh, my bad, I see that this code indeed already covers the other points I mentioned (about masking for calculating err, etc). Sorry, I had overlooked some lines and didn't actually check the unit tests!

However, one issue: test_combine.py only calls one of the tests to run after if __name__ == '__main__'.

[semaphoreP]

the pytest framework ignores code in if __name__ == '__main__'. That chunk of code is only used for developer debugging (i.e., running it in a debugger). All pytest evocations will run every defined function that starts with the "test_" in its name.

[kjl0025]

Ah, okay, thanks!