# Reduce ISR code duplication between ip_isr, obs_subaru, and obs_decam

XMLWordPrintable

#### Details

• Type: Improvement
• Status: Done
• Resolution: Done
• Fix Version/s: None
• Component/s:
• Labels:
None
• Story Points:
8
• Sprint:
DRP F18-5, DRP F18-6, DRP S19-1
• Team:
Data Release Production

#### Description

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.

#### Activity

Hide
Merlin Fisher-Levine added a comment -

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.

Show
Merlin Fisher-Levine added a comment - 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.
Hide
Merlin Fisher-Levine added a comment -

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.

Show
Merlin Fisher-Levine added a comment - 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.
Hide
Christopher Waters added a comment -

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.

Show
Christopher Waters added a comment - 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.
Hide
Merlin Fisher-Levine added a comment -

Oh yes, the benefits from this will be many

Show
Merlin Fisher-Levine added a comment - Oh yes, the benefits from this will be many
Hide
Christopher Waters added a comment -

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.

Show
Christopher Waters added a comment - 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.
Hide
Simon Krughoff added a comment -

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.

Show
Simon Krughoff added a comment - 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.

#### People

Assignee:
Christopher Waters
Reporter:
Christopher Waters
Reviewers:
Simon Krughoff
Watchers:
Christopher Waters, Meredith Rawls, Merlin Fisher-Levine, Simon Krughoff