Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-15862

Reduce ISR code duplication between ip_isr, obs_subaru, and obs_decam

    Details

    • Type: Improvement
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: ip_isr, obs_decam, obs_subaru
    • 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.

        Attachments

          Issue Links

            Activity

            Hide
            mfisherlevine 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
            mfisherlevine 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
            mfisherlevine 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
            mfisherlevine 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
            czw 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
            czw 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
            mfisherlevine Merlin Fisher-Levine added a comment -

            Oh yes, the benefits from this will be many

            Show
            mfisherlevine Merlin Fisher-Levine added a comment - Oh yes, the benefits from this will be many
            Hide
            czw 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
            czw 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
            krughoff 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
            krughoff 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:
                czw Christopher Waters
                Reporter:
                czw Christopher Waters
                Reviewers:
                Simon Krughoff
                Watchers:
                Christopher Waters, Meredith Rawls, Merlin Fisher-Levine, Simon Krughoff
              • Votes:
                0 Vote for this issue
                Watchers:
                4 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel