Details
-
Type:
Story
-
Status: Done
-
Resolution: Done
-
Fix Version/s: None
-
Component/s: ip_isr
-
Labels:None
-
Story Points:5
-
Epic Link:
-
Team:Alert Production
Description
In the default IsrTask, the variance is currently set after dark subtraction. This means that photon noise from the dark is not included in the variance plane, which is incorrect. The variance should be set after bias subtraction and before dark subtraction.
Hsin-Fang Chiang also points out (DM-4191) that the AssembleCcdTask with default parameters requires amplifier images with variance planes, even though the variance cannot be set properly until after full-frame bias subtraction. I believe that AssembleCcdTask only requires a variance plane in the amp images because it does an "effective gain" calculation, but I suggest that this isn't very useful (an approximation of an approximation, and you're never going to use that information anyway because it's embedded in the variance plane with better fidelity). I therefore suggest that this effective gain calculation be stripped out and that AssembleCcdTask not require variance planes.
Attachments
Issue Links
- is triggered by
-
- Implemented
Activity
The changes themselves looks fine, but I wish you would modify AssembleCoaddTask to not require variance planes and remove the effective gain calculation, as argued in the ticket description.
Paul Price
added a comment - The changes themselves looks fine, but I wish you would modify AssembleCoaddTask to not require variance planes and remove the effective gain calculation, as argued in the ticket description. I wish you would modify AssembleCoaddTask to not require variance planes and remove the effective gain calculation, as argued in the ticket description.
I am fine with this. Do we even need to keep the effective gain calculation? It's not used anywhere as far as I can tell. Most configs have that set option set to False anyway.
Next question, since removing the gain calculation changes the API, doesn't it need an RFC?
Yes, I think it would. Sorry!
Paul Price
added a comment - Yes, I think it would. Sorry! Who added the effective gain (possibly me)? It'd be good to understand why it was added before removing it.
Does it make sense to merge this ticket as is, then file an RFC?
I think I have implemented the requests in this ticket. Hsin-Fang Chiang will you have a look please? There are some persisted configs in validate_* packages that still contain the removed config parameters, but those will be replaced once we re-run them.
Looks good to me. Thanks for fixing this.
Just a reminder, please run the CIs (including validate_drp/lsst_ci that may not run automatically in the Jenkins) before merging.
Hsin-Fang Chiang
added a comment - Looks good to me. Thanks for fixing this.
Just a reminder, please run the CIs (including validate_drp/lsst_ci that may not run automatically in the Jenkins) before merging. Just perusing this ticket. Paul Price, is the following no longer true given the removal of setGain()?
https://github.com/lsst/obs_subaru/blob/master/python/lsst/obs/subaru/isr.py#L271
I.e., should the variance plane setting here now be moved to after bias subtraction?
Lauren MacArthur good catch. I had removed the config parameters from the overrides, but didn't see that bit. I can change it on this ticket, if you want.
Paul Price or Lauren MacArthur will one of you look at the change I made in this PR?
It looks good to me, but I still think it should get Paul Price's eyes on it.
O.K. I found an error when I ran it through ci_hsc, but my commit this morning fixed it, so ci_hsc now passes.
O.K. ci_hsc and validate_drp both passed, but validate_drp only took 18 sec. Should I be worried?
Edit: Sorry I was confused. validate_drp and ci_hsc are not executed in the same way. validate_drp is currently down, but should be back up soon.
validate_drp doesn't do much when you build it. You have to run it to see what happens. I suggest you run the demo for CFHT using the code before your changes and after to see what changes (if anything). You could run the DECam demo as well, but I suspect it does not run ISR.
O.K. so I can't run validate_drp against a ticket branch, but ci_hsc passes, so I think that means my changes are o.k.
If I don't hear a lot of shouting from Paul Price I'm going to merge this week.
The changes to obs_subaru look fine, thanks. Just a couple of comments on the GitHub PR to clean things up.
Paul Price
added a comment - The changes to obs_subaru look fine, thanks. Just a couple of comments on the GitHub PR to clean things up. Merged
Paul Price as reporter, I'd like to ask you to review this very small changeset. Let me know if you'd prefer not to do it.