Fix Version/s: None
Component/s: ip_isr, obs_decam, obs_subaru
Sprint:DRP F18-5, DRP F18-6, DRP S19-1
Team:Data Release Production
In sorting out tests for `ip_isr`, I've noticed that there is duplicated code between that package and `obs_subaru`. There is additional ISR related code in `obs_decam`. The goal is to unify as much as possible into the default `ip_isr` processing, with the `obs_*` packages only modifying methods when needed.
DM-14136 convert ip_isr documentation to numpydoc
- To Do
DM-16467 isrTask conversion to pipelineTask
- is duplicated by
DM-15161 HSC should use the generic IsrTask from ip_isr
- is FF-depended by
DM-16802 ISR order-of-operations not uniquely defined
- Won't Fix
- is triggered by
RFC-544 ISR rewrites for reduced code duplication
DM-9025 IsrTask always processes "raw" dataset type despite having a config parameter
And just to be clear, I think that isr has been much more thoroughly tested on the HSC side than anywhere else, so bringing that into the main isr is great, and people should therefore benefit from these isr changes, but people should also be made aware that some things may not be bitwise identical after these changes, I think, otherwise they might freak out.
I agree on all points. My hope is that this will be relatively seamless for the other `obs_*` packages, other than increased set of configuration options to enable/disable new options to keep the same output behavior.
The side benefit of this change is that my pytest updates for `ip_isr` (
DM-15683) using HSC data will directly test the generic ISR without swapping in `obs_subaru` methods.
I'm not sure this is truly a complete solution (see
RFC-544 for discussion of stage ordering), but I would like to get this ticket merged, and deal with anyISR reordering as a separate issue. The ISR stages should be easy to rearrange from this starting point, and it should hopefully only require changes to `ip_isr`.
I've asked for changes on most of the PRs, but, to be honest, it is mostly cleanup work that I hope is easy to take care of.
I will say (as I said on the review of ip_isr) that I did not invest a lot of effort in a detailed review of the algorithmic code as it seems to be mostly brought from other places and it looked generally ok anyway.
Before this lands, I think you should recruit someone in AP to run the ap_verify on, at least, the CI dataset so they can make sure the changes in e.g. order of operations do not make bigger than expected impacts.
This, as I'm sure you're aware, is potentially quite a thorny thing to do.
I imagine the end goal is to have the vanilla isr work as much like the Subaru one as possible right, and make sure that the behaviour on the Subaru side is identical. Is that roughly right?
I don't want to hamstring this at all because I think it really needs doing and will be a huge win in a lot of ways, but this has the potential to impact isr behaviour in all other obs_packages as (I assume) you'll be changing the nominal implementation to match the current HSC one, so I think it's good to be mindful of how we will measure changes to the performance of the current isr behaviour somehow.