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

Clean up code in afw for Approximate background estimation

    Details

    • Type: Improvement
    • Status: Invalid
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: afw
    • Labels:
      None

      Description

      The intention is to eventually set useApprox=True (i.e. Chebychev Approximation) as the default for background estimation. However, in looking into the relevant code in afw/math while working on DM-2778, there is some clean-up and restructuring that needs to be done before resetting the defaults (which may also require adjusting some defaults in the calibrate stage to be more appropriate for the approximation, as opposed to inperpolation, scheme). This issue is to clean up the code and make sure it all operates coherently. A seperate ticket will be to actually reset the defaults and make any other config default changes required.

      In particular, the config setting of approxOrderX/binSize are not being assessed properly, nor is the behavior of the given undersampleStyle being executed. The under-sampling checks are currently only being done against the interpoation settings, which is not appropriate when useApprox=True. A temporary check was added in meas.algoritms.detection.getBackground() in DM-2778 so that it is currently "safe" to run with useApprox=True and any other user overridden config setting (binSize, approxOrder, undersampleStyle) (and there currently exists similar checks in pipe.tasks.matchBackground), but these should be removed once this issue has been implemented.

        Attachments

          Issue Links

            Activity

            lauren Lauren MacArthur created issue -
            lauren Lauren MacArthur made changes -
            Field Original Value New Value
            Link This issue blocks DM-2921 [ DM-2921 ]
            lauren Lauren MacArthur made changes -
            Description The intention is to eventually set useApprox=True (i.e. Chebychev Approximation) as the default for background estimation. However, in looking into the relevant code in afw/math while working on DM-2778, there is some clean-up and restructuring that needs to be done before resetting the defaults (which may also require adjusting some defaults in the calibrate stage to be more appropriate for the approximation, as opposed to inperpolation, scheme). This issue is to clean up the code and make sure it all operates coherently. A seperate ticket will be to actually reset the defaults and make any other config default changes required.

            In particular, the config setting of approxOrder/binSize are not being assessed properly, nor is the behavior of the given undersampleStlye being executed. The under-sampling checks are currently only being done against the interpoation settings, which is not appropriate when useApprox=True. A temporary check was added meas.algoritms.detection.getBackground() in DM-2778 so that it is currently "safe" to run with useApprox=True and any other user overridden config setting (binSize, approxOrder, undersampleStyle) (and there currently exists similar checks in pipe.tasks.matchBackground), but these should be removed once this issue has been implemented.
            The intention is to eventually set {{useApprox=True}} (i.e. Chebychev Approximation) as the default for background estimation. However, in looking into the relevant code in afw/math while working on DM-2778, there is some clean-up and restructuring that needs to be done before resetting the defaults (which may also require adjusting some defaults in the calibrate stage to be more appropriate for the approximation, as opposed to inperpolation, scheme). This issue is to clean up the code and make sure it all operates coherently. A seperate ticket will be to actually reset the defaults and make any other config default changes required.

            In particular, the config setting of approxOrderX/binSize are not being assessed properly, nor is the behavior of the given undersampleStlye being executed. The under-sampling checks are currently only being done against the interpoation settings, which is not appropriate when useApprox=True. A temporary check was added in {{meas.algoritms.detection.getBackground()}} in DM-2778 so that it is currently "safe" to run with useApprox=True and any other user overridden config setting (binSize, approxOrder, undersampleStyle) (and there currently exists similar checks in {{pipe.tasks.matchBackground}}), but these should be removed once this issue has been implemented.
            swinbank John Swinbank made changes -
            Epic Link DM-1907 [ 15927 ]
            swinbank John Swinbank made changes -
            Description The intention is to eventually set {{useApprox=True}} (i.e. Chebychev Approximation) as the default for background estimation. However, in looking into the relevant code in afw/math while working on DM-2778, there is some clean-up and restructuring that needs to be done before resetting the defaults (which may also require adjusting some defaults in the calibrate stage to be more appropriate for the approximation, as opposed to inperpolation, scheme). This issue is to clean up the code and make sure it all operates coherently. A seperate ticket will be to actually reset the defaults and make any other config default changes required.

            In particular, the config setting of approxOrderX/binSize are not being assessed properly, nor is the behavior of the given undersampleStlye being executed. The under-sampling checks are currently only being done against the interpoation settings, which is not appropriate when useApprox=True. A temporary check was added in {{meas.algoritms.detection.getBackground()}} in DM-2778 so that it is currently "safe" to run with useApprox=True and any other user overridden config setting (binSize, approxOrder, undersampleStyle) (and there currently exists similar checks in {{pipe.tasks.matchBackground}}), but these should be removed once this issue has been implemented.
            The intention is to eventually set {{useApprox=True}} (i.e. Chebychev Approximation) as the default for background estimation. However, in looking into the relevant code in afw/math while working on DM-2778, there is some clean-up and restructuring that needs to be done before resetting the defaults (which may also require adjusting some defaults in the calibrate stage to be more appropriate for the approximation, as opposed to inperpolation, scheme). This issue is to clean up the code and make sure it all operates coherently. A seperate ticket will be to actually reset the defaults and make any other config default changes required.

            In particular, the config setting of approxOrderX/binSize are not being assessed properly, nor is the behavior of the given undersampleStyle being executed. The under-sampling checks are currently only being done against the interpoation settings, which is not appropriate when useApprox=True. A temporary check was added in {{meas.algoritms.detection.getBackground()}} in DM-2778 so that it is currently "safe" to run with useApprox=True and any other user overridden config setting (binSize, approxOrder, undersampleStyle) (and there currently exists similar checks in {{pipe.tasks.matchBackground}}), but these should be removed once this issue has been implemented.
            Hide
            swinbank John Swinbank added a comment -

            Is this something that Nate Lust could look at? It sounds as though it would give him a nice opportunity to exercise his C++ without requiring too much knowledge of the wider stack.

            Show
            swinbank John Swinbank added a comment - Is this something that Nate Lust could look at? It sounds as though it would give him a nice opportunity to exercise his C++ without requiring too much knowledge of the wider stack.
            Hide
            lauren Lauren MacArthur added a comment -

            John Swinbank Indeed. Nate Lust and I just discussed it and we plan to look at it in more detail on Monday.

            Show
            lauren Lauren MacArthur added a comment - John Swinbank Indeed. Nate Lust and I just discussed it and we plan to look at it in more detail on Monday.
            swinbank John Swinbank made changes -
            Assignee Nate Lust [ nlust ]
            swinbank John Swinbank made changes -
            Epic Link DM-1907 [ 15927 ] DM-3561 [ 19651 ]
            swinbank John Swinbank made changes -
            Epic Link DM-3561 [ 19651 ] DM-3568 [ 19662 ]
            swinbank John Swinbank made changes -
            Assignee Nate Lust [ nlust ] Vishal Kasliwal [ vpk24 ]
            Hide
            nlust Nate Lust added a comment -

            Jim Bosch I seem to remember you telling Lauren MacArthur and I that this ticket and 2921 were not to be worked on. I seem to remember you saying the code was going to be refactored or superseded in the near future. Can you confirm that Lauren and my memory is correct?

            Show
            nlust Nate Lust added a comment - Jim Bosch I seem to remember you telling Lauren MacArthur and I that this ticket and 2921 were not to be worked on. I seem to remember you saying the code was going to be refactored or superseded in the near future. Can you confirm that Lauren and my memory is correct?
            Hide
            swinbank John Swinbank added a comment -

            Nate Lust queries whether this ticket is still relevant given the planned effort on DM-1991. I guess there are a couple of considerations:

            • On what timescale is DM-1991 likely to be completed?
            • Does the work on DM-1991 planned to be an entirely new implementation, or will it make use of what we already have? If the latter, presumably fixing up the existing implementation is worthwhile.

            Simon Krughoff, Yusra AlSayyad: do you have any thoughts?

            Show
            swinbank John Swinbank added a comment - Nate Lust queries whether this ticket is still relevant given the planned effort on DM-1991 . I guess there are a couple of considerations: On what timescale is DM-1991 likely to be completed? Does the work on DM-1991 planned to be an entirely new implementation, or will it make use of what we already have? If the latter, presumably fixing up the existing implementation is worthwhile. Simon Krughoff , Yusra AlSayyad : do you have any thoughts?
            Hide
            yusra Yusra AlSayyad added a comment - - edited

            The plan is to retire Approximate which was replaced with ChebyshevBoundedField. So before you make substantial changes to Approximate, know that that code won't be around for long.

            If you need to default to useApprox=True now, and want your work survive DM-1991 you could edit background to use ChebyshevBoundedField instead of Approximate.

            The work on DM-1991 will not be an entirely new implementation. What is left on DM-1991 is:

            • Write a SplineBoundedField to replace BackgroundMI.
            • Make some minor API changes to the BoundedField family, including replacing the name BoundedField with Model2D (unless everyone is getting attached to BoundedField)
            • Edit Background to use SplineBoundedField and ChebyshevBoundedField
            • Delete Approximate/BackgroundMI

            As far as timing goes, I will not have time to look at this until after Nov 2nd. Has all the HSC code that depends on the original BoundedField been ported over?

            Show
            yusra Yusra AlSayyad added a comment - - edited The plan is to retire Approximate which was replaced with ChebyshevBoundedField . So before you make substantial changes to Approximate, know that that code won't be around for long. If you need to default to useApprox=True now, and want your work survive DM-1991 you could edit background to use ChebyshevBoundedField instead of Approximate. The work on DM-1991 will not be an entirely new implementation. What is left on DM-1991 is: Write a SplineBoundedField to replace BackgroundMI . Make some minor API changes to the BoundedField family, including replacing the name BoundedField with Model2D (unless everyone is getting attached to BoundedField) Edit Background to use SplineBoundedField and ChebyshevBoundedField Delete Approximate/BackgroundMI As far as timing goes, I will not have time to look at this until after Nov 2nd. Has all the HSC code that depends on the original BoundedField been ported over?
            Hide
            swinbank John Swinbank added a comment -

            Thanks Yusra AlSayyad. My reading of the above is that there is no point in proceeding with work on the old codebase, so we can drop this issue. Good catch, Nate Lust. I don't think there's desperate urgency to adopt useApprox=True immediately, but, if such a requirement were to arise, I guess the best approach would be for us to devote some effort to helping with DM-2478. For now, I think we can close this and DM-2921 as invalid – if anybody disagrees, please feel free to reopen.

            Has all the HSC code that depends on the original BoundedField been ported over?

            I confess that I don't know the answer to this, but I'm not sure that it should make a substantial difference: we'll either need to update it to use the new BoundedField/Model2D API when it's ported (if the API is available) or at some future date (if it isn't), so I don't think it should interfere with your schedule.

            Show
            swinbank John Swinbank added a comment - Thanks Yusra AlSayyad . My reading of the above is that there is no point in proceeding with work on the old codebase, so we can drop this issue. Good catch, Nate Lust . I don't think there's desperate urgency to adopt useApprox=True immediately, but, if such a requirement were to arise, I guess the best approach would be for us to devote some effort to helping with DM-2478 . For now, I think we can close this and DM-2921 as invalid – if anybody disagrees, please feel free to reopen. Has all the HSC code that depends on the original BoundedField been ported over? I confess that I don't know the answer to this, but I'm not sure that it should make a substantial difference: we'll either need to update it to use the new BoundedField / Model2D API when it's ported (if the API is available) or at some future date (if it isn't), so I don't think it should interfere with your schedule.
            swinbank John Swinbank made changes -
            Resolution Done [ 10000 ]
            Status To Do [ 10001 ] Invalid [ 11005 ]
            Hide
            vpk24 Vishal Kasliwal [X] (Inactive) added a comment -

            Ok, I'll start watching DM-1991 instead. Good catch Nate!

            Show
            vpk24 Vishal Kasliwal [X] (Inactive) added a comment - Ok, I'll start watching DM-1991 instead. Good catch Nate!

              People

              • Assignee:
                vpk24 Vishal Kasliwal [X] (Inactive)
                Reporter:
                lauren Lauren MacArthur
                Watchers:
                John Swinbank, Lauren MacArthur, Nate Lust, Vishal Kasliwal [X] (Inactive), Yusra AlSayyad
              • Votes:
                0 Vote for this issue
                Watchers:
                5 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel