Photon counting #277
Photon counting #277
Conversation
… fixed a minor error in combine.py almost finished with unit tests for photon counting
so that I can leave the "OPTIONAL" out of the recipe in prescan subtraction since we do want the calibrated bias offset ideally
…tests when it runs
…ssed minor issues 200 and 244
|
@kjl0025 can you finish filling out the DRP Test Definition row with your test description. It'll actually be helpful for my review to be able to read through the tests at a high level. (Rather than waiting to fill it out after PR review). |
…-checkbox Pr template update with test checkbox
|
Sure, just finished doing that. |
There was a problem hiding this comment.
Hi Kevin, thanks for putting this together. I put some comments throughout.
Some of the other e2e tests (e.g., test_astrom) are failing. I think it's because of the changes to Kgain requiring RN and RN_ERR. Can you fix them? One possibility while fixing them is editing the creation of Kgain so that RN and RN_ERR are things that get defined in it's __init__() function (so it's never missing, or it throws are error then). Given RN and RN_ERR are now required, it seems like good practice to force the definition of them there.
Happy to discuss other comments (I think some other moderate sized comments are to add some explicit flag to toggle between PC dark creation and PC dark subtraction in the step function, and to make sure your PC recipe goes all the way to the end of L2b processing).
|
Alright, I think I've addressed all your comments. Should be good to go. |
|
Should be good to go now. |
|
Oops, not yet. Fixing |
'ISPC' is not currently in any files; it will be added, though
…idrp into photon_counting
With the addition of simulation of smearing, the tolerance needed to be increased slightly
There was a problem hiding this comment.
Hi Kevin, just a few small changes in the code, to conform to some python best practices and hopefully avoid bugs in future development.
Additionally, it seems like header keywords for CMDGAIN and KGAIN have changed. Can you make a new issue to track that we should update these header keywords? I would strongly advocate not doing it in this PR so we can wrap up the last few things and merge!
corgidrp/walker.py
Outdated
| elif image.pri_hdr['VISTYPE'] == "PUPILIMG": | ||
| recipe_filename = ["l1_to_l2a_nonlin.json", "l1_to_kgain.json"] | ||
| else: | ||
| recipe_filename = "l1_to_l2b.json" | ||
| if 'ISPC' in image.ext_hdr: |
There was a problem hiding this comment.
can this be an elif before the else above? I'd like to keep the logic clean (and easier to debug) using else statements whenever possible.
corgidrp/walker.py
Outdated
| recipe_filename = "l1_to_l2b_pc_dark.json" | ||
| elif len(unique_vals) > 1: # darks for noisemap creation | ||
| recipe_filename = "l1_to_l2a_noisemap.json" | ||
| elif len(unique_vals) == 1: # traditional darks |
There was a problem hiding this comment.
We shouldn't end on an if. We should turn this into an else or have an else below that raises an Exception.
Same for the 3 other places you have this structure.
also set the number of frames in photon_count_e2e.py back to what it should be. (I set the number of frames to 2 for quick testing.)
Describe your changes
Added a step function for photon counting frames. Also addressed some other minor issues. (This is the same as the PR I had into the main branch.)
Type of change
Please delete options that are not relevant (and this line).
Reference any relevant issues (don't forget the #)
#2 , #200 , #244
Checklist before requesting a review