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

HSC backport: allow for use of Approximate model in background estimation

    XMLWordPrintable

    Details

    • Story Points:
      6
    • Sprint:
      Science Pipelines DM-S15-3, Science Pipelines DM-S15-4
    • Team:
      Data Release Production

      Description

      This issue involves transferring changesets from the following HSC issues:

      • HSC-145 Investigate approximating rather than interpolating backgrounds
      • HSC-1047 Background object cannot be loaded with butler
      • HSC-1213 Set background 'approximate' control settings when background control is created.
      • HSC-1221 tests failing in ip_diffim
      • HSC-1217 Verify backgroundList IO works properly when Approximate is enabled in background control - HSC JIRA

      The Approximate (Chebyshev) approach greatly improves the background subtraction around bright objects compared with the interpolation scheme currently in use (which over-subtracts near bright objects).

        Attachments

          Issue Links

            Activity

            No builds found.
            lauren Lauren MacArthur created issue -
            Hide
            lauren Lauren MacArthur added a comment -

            Simon Krughoff & Yusra AlSayyad: shortly after creating this ticket, Robert Lupton reminded us that you have some tasks in LDM-240 to clean up background estimation, so I wanted to check with you before implementing this ticket. Please let me know if you have any concerns and/or objections.

            Show
            lauren Lauren MacArthur added a comment - Simon Krughoff & Yusra AlSayyad : shortly after creating this ticket, Robert Lupton reminded us that you have some tasks in LDM-240 to clean up background estimation, so I wanted to check with you before implementing this ticket. Please let me know if you have any concerns and/or objections.
            Hide
            yusra Yusra AlSayyad added a comment -

            I'm very happy to see this issue. I wasn't aware of any tasks to clean up background estimation this year. But for the June 2015 sprint, we were planning on refactoring Approximate/Interpolate. This will look very similar to HSC's BoundedField. You can implement this ticket with the current Approximate API on the LSST side though.

            Show
            yusra Yusra AlSayyad added a comment - I'm very happy to see this issue. I wasn't aware of any tasks to clean up background estimation this year. But for the June 2015 sprint, we were planning on refactoring Approximate/Interpolate. This will look very similar to HSC's BoundedField. You can implement this ticket with the current Approximate API on the LSST side though.
            lauren Lauren MacArthur made changes -
            Field Original Value New Value
            Status To Do [ 10001 ] In Progress [ 3 ]
            Hide
            lauren Lauren MacArthur added a comment -

            Also added the following commit:

            commit 1d4ffae208574b210a627a473f3d63a0b09d9825
            Author: Robert Lupton the Good <rhl@astro.princeton.edu>
            Date:   Tue Feb 26 06:06:52 2013 +0900
             
                Support full background models when detecting cosmic rays
                
                Needed for e.g. M31
             
             python/lsst/meas/algorithms/findCosmicRaysConfig.py | 12 ++++++++++++
             1 file changed, 12 insertions(+)
            

            Show
            lauren Lauren MacArthur added a comment - Also added the following commit: commit 1d4ffae208574b210a627a473f3d63a0b09d9825 Author: Robert Lupton the Good <rhl@astro.princeton.edu> Date: Tue Feb 26 06:06:52 2013 +0900   Support full background models when detecting cosmic rays Needed for e.g. M31   python/lsst/meas/algorithms/findCosmicRaysConfig.py | 12 ++++++++++++ 1 file changed, 12 insertions(+)
            Hide
            krughoff Simon Krughoff added a comment -

            Lauren MacArthur we have the task to implement full focalplane background modeling. This is not on the roadmap for 2015, but is staged to do in 2016.

            Show
            krughoff Simon Krughoff added a comment - Lauren MacArthur we have the task to implement full focalplane background modeling. This is not on the roadmap for 2015, but is staged to do in 2016.
            lauren Lauren MacArthur made changes -
            Attachment Interp_vs_Approx.png [ 26907 ]
            Hide
            lauren Lauren MacArthur added a comment -

            Thanks Simon Krughoff & Yusra AlSayyad. I've pulled over the changes. I'd say the attached before (left) and after (right) is argument enough that it's a definite improvement!

            I'm now trying to resolve some test failures in pipe_tasks.

            Show
            lauren Lauren MacArthur added a comment - Thanks Simon Krughoff & Yusra AlSayyad . I've pulled over the changes. I'd say the attached before (left) and after (right) is argument enough that it's a definite improvement! I'm now trying to resolve some test failures in pipe_tasks.
            Hide
            price Paul Price added a comment -

            There were some changes to tests that Steve put in after he merged to master, maybe in another package as well, so you should probably run buildbot. The fix might have been on another HSC ticket.

            Show
            price Paul Price added a comment - There were some changes to tests that Steve put in after he merged to master, maybe in another package as well, so you should probably run buildbot. The fix might have been on another HSC ticket.
            Hide
            lauren Lauren MacArthur added a comment -

            Paul Price Yeah, I got at least one of Steve's fixes for the test failure in ip_diffim (HSC-1221). I did run a Buildbot, which is how I discovered these pipe_task test failures. I'm actually now trying to configure the weighting parameter you added to the Approximate class on LSST (that addition did not make it to HSC & the currently failing tests don't exist in HSC's pipe_tasks). I think some of these tests may use test data with wonky variance planes, so I'm seeing if setting weighting=False solves the issue. The other solution would be to have these tests run with useApprox=False (which is effectively how they ran prior to this), but I'm hoping to get both cases passing!

            Show
            lauren Lauren MacArthur added a comment - Paul Price Yeah, I got at least one of Steve's fixes for the test failure in ip_diffim (HSC-1221). I did run a Buildbot, which is how I discovered these pipe_task test failures. I'm actually now trying to configure the weighting parameter you added to the Approximate class on LSST (that addition did not make it to HSC & the currently failing tests don't exist in HSC's pipe_tasks). I think some of these tests may use test data with wonky variance planes, so I'm seeing if setting weighting=False solves the issue. The other solution would be to have these tests run with useApprox=False (which is effectively how they ran prior to this), but I'm hoping to get both cases passing!
            swinbank John Swinbank made changes -
            Epic Link DM-1907 [ 15927 ]
            swinbank John Swinbank made changes -
            Link This issue blocks DM-1945 [ DM-1945 ]
            swinbank John Swinbank made changes -
            Sprint Science Pipelines DM-S15-3 [ 155 ]
            krughoff Simon Krughoff made changes -
            Sprint Science Pipelines DM-S15-3 [ 155 ] Science Pipelines DM-S15-3, Science Pipelines DM-S15-4 [ 155, 159 ]
            krughoff Simon Krughoff made changes -
            Rank Ranked higher
            Hide
            lauren Lauren MacArthur added a comment -

            When using the Approximate background estimation as the default, the output in the lsst_dm_stack_demo run differs from the current "expected" files that were run using the interpolated background default. If we are to set useApprox = True as the default, the "expected" output files in the lsst_dm_stack_demo repository will need to be replaced. Before considering this change, as a sanity check I attach here a comparison between "matched" sources between the current "expected" files and the output from a useApprox = True run. By "matched", I mean the output from running afwTable.matchRaDec() on the two catalogs.

            Also note that the binSize's in the obs_sdss processCcd.py config file had to be decreased from 512 to 128 as the former is too coarse for the Approximate background estimation. If useApprox = True is set as the new default for background estimation, there are a few default binSize's throughout the stack that will need to be adjusted to smaller sizes more appropriate for the Chebyshev approximation.

            Please post any comments/concerns/requests for further details.

            Show
            lauren Lauren MacArthur added a comment - When using the Approximate background estimation as the default, the output in the lsst_dm_stack_demo run differs from the current "expected" files that were run using the interpolated background default. If we are to set useApprox = True as the default, the "expected" output files in the lsst_dm_stack_demo repository will need to be replaced. Before considering this change, as a sanity check I attach here a comparison between "matched" sources between the current "expected" files and the output from a useApprox = True run. By "matched", I mean the output from running afwTable.matchRaDec() on the two catalogs. Also note that the binSize's in the obs_sdss processCcd.py config file had to be decreased from 512 to 128 as the former is too coarse for the Approximate background estimation. If useApprox = True is set as the new default for background estimation, there are a few default binSize's throughout the stack that will need to be adjusted to smaller sizes more appropriate for the Chebyshev approximation. Please post any comments/concerns/requests for further details.
            lauren Lauren MacArthur made changes -
            Attachment lin_base_PsfFlux_flux.png [ 26912 ]
            Attachment lin_base_GaussianFlux_flux.png [ 26913 ]
            Attachment diff_base_GaussianFlux_flux.png [ 26914 ]
            Attachment diff_base_PsfFlux_flux.png [ 26915 ]
            lauren Lauren MacArthur made changes -
            Story Points 2 6
            jbosch Jim Bosch made changes -
            Description This issue involves transferring changesets from the following HSC issues:

            - [HSC-145|https://hsc-jira.astro.princeton.edu/jira/browse/HSC-145] Investigate approximating rather than interpolating backgrounds
            - [HSC-1047|https://hsc-jira.astro.princeton.edu/jira/browse/HSC-1047] Background object cannot be loaded with butler
            - [HSC-1213|https://hsc-jira.astro.princeton.edu/jira/browse/HSC-1213] Set background 'approximate' control settings when background control is created.
            - [HSC-1221|https://hsc-jira.astro.princeton.edu/jira/browse/HSC-1221] tests failing in ip_diffim

            The Approximate (Chebyshev) approach greatly improves the background subtraction around bright objects compared with the interpolation scheme currently in use (which over-subtracts near bright objects).
            This issue involves transferring changesets from the following HSC issues:

            - [HSC-145|https://hsc-jira.astro.princeton.edu/jira/browse/HSC-145] Investigate approximating rather than interpolating backgrounds
            - [HSC-1047|https://hsc-jira.astro.princeton.edu/jira/browse/HSC-1047] Background object cannot be loaded with butler
            - [HSC-1213|https://hsc-jira.astro.princeton.edu/jira/browse/HSC-1213] Set background 'approximate' control settings when background control is created.
            - [HSC-1221|https://hsc-jira.astro.princeton.edu/jira/browse/HSC-1221] tests failing in ip_diffim
            - [HSC-1217|https://hsc-jira.astro.princeton.edu/jira/browse/HSC-1217] Verify backgroundList IO works properly when Approximate is enabled in background control - HSC JIRA

            The Approximate (Chebyshev) approach greatly improves the background subtraction around bright objects compared with the interpolation scheme currently in use (which over-subtracts near bright objects).
            Hide
            lauren Lauren MacArthur added a comment -

            In looking into the relevant code in afw/math while working on this issue, it is clear 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). Therefore, the default setting will not be changed as part of this ticket (i.e. useApprox is set to False). A separate issue (DM-2920) has been created to clean up the code and make sure it all operates coherently. Another ticket (DM-2921) has been created to actually reset the defaults and make any other config default changes required.

            In the meantime, a temporary check has been added here in meas.algoritms.detection.getBackground() so that it is currently "safe" to run with useApprox=True and any other user overridden config setting (binSize, approxOrderX, undersampleStyle) (and there currently exists similar checks in pipe.tasks.matchBackground), but these should be removed once DM-2920 has been implemented.

            Show
            lauren Lauren MacArthur added a comment - In looking into the relevant code in afw/math while working on this issue, it is clear 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). Therefore, the default setting will not be changed as part of this ticket (i.e. useApprox is set to False). A separate issue ( DM-2920 ) has been created to clean up the code and make sure it all operates coherently. Another ticket ( DM-2921 ) has been created to actually reset the defaults and make any other config default changes required. In the meantime, a temporary check has been added here in meas.algoritms.detection.getBackground() so that it is currently "safe" to run with useApprox=True and any other user overridden config setting (binSize, approxOrderX, undersampleStyle) (and there currently exists similar checks in pipe.tasks.matchBackground ), but these should be removed once DM-2920 has been implemented.
            lauren Lauren MacArthur made changes -
            Component/s pipe_tasks [ 10726 ]
            Component/s obs_subaru [ 10747 ]
            Hide
            lauren Lauren MacArthur added a comment -

            Yusra, would you mind having a look at this? The relevant commits are all in u/lauren/DM-2778 branches on the following repos:

            • afw
            • meas_algorithms
            • pipe_tasks
            • ip_diffim

            I ran a Buildbot against them and it was successful. I've also put running with useApprox=True through its paces with various config settings using testProcessCcd.py in pipe_tasks and have processed some real data (see attached figures). Formal unittests await a refactoring of some code in DM-2920.

            Show
            lauren Lauren MacArthur added a comment - Yusra, would you mind having a look at this? The relevant commits are all in u/lauren/ DM-2778 branches on the following repos: afw meas_algorithms pipe_tasks ip_diffim I ran a Buildbot against them and it was successful. I've also put running with useApprox=True through its paces with various config settings using testProcessCcd.py in pipe_tasks and have processed some real data (see attached figures). Formal unittests await a refactoring of some code in DM-2920 .
            lauren Lauren MacArthur made changes -
            Reviewers Yusra AlSayyad [ yusra ]
            Status In Progress [ 3 ] In Review [ 10004 ]
            Hide
            yusra Yusra AlSayyad added a comment -

            Looks good. Couple minor questions/comments:

            pipe_tasks/meas_algorithms:
            In MatchBackgroundsConfig there is a config parameter called "usePolynomial" which is used the same way that "useApprox" is being used in meas_algorithms. We'll want eventual consistency, but it is not necessary on this ticket. I'm not sure useApprox is all that transparent to Task users.
            python/lsst/meas/algorithms/detection.py: Double check that the comment starting at line 579 is current.

            afw:
            Are we temporarily losing the ability to do spline interpolation on doubles? I'm not too worried though since we will get this back with a stand alone interpolate2D class.

            include/lsst/afw/math/Approximate.h: line 59-68 getters have comments but setters don't. I don't necessarily think the setters need comments, but I'm realizing our styles in header files are diverging.

            approximate.i: Copyleft in brand new files says 2008, 2009, 2010? (I see tests/background.py was updated on this ticket to use 2008-2015 AURA/LSST)

            python/lsst/afw/math/background.py:
            *I like that there is a section for backwards compatibility here. Will we be supporting these new property set keys when they change in the future?

            • line 171/172. ApproxOrderX/Y defaults when not useApprox: Please add a comment as to why is one default is -1 and the other +1.
            • 809/820 commented-out print statements

            ip_diffim: look good; no comments.

            Show
            yusra Yusra AlSayyad added a comment - Looks good. Couple minor questions/comments: pipe_tasks/meas_algorithms: In MatchBackgroundsConfig there is a config parameter called "usePolynomial" which is used the same way that "useApprox" is being used in meas_algorithms. We'll want eventual consistency, but it is not necessary on this ticket. I'm not sure useApprox is all that transparent to Task users. python/lsst/meas/algorithms/detection.py : Double check that the comment starting at line 579 is current. afw: Are we temporarily losing the ability to do spline interpolation on doubles? I'm not too worried though since we will get this back with a stand alone interpolate2D class. include/lsst/afw/math/Approximate.h: line 59-68 getters have comments but setters don't. I don't necessarily think the setters need comments, but I'm realizing our styles in header files are diverging. approximate.i: Copyleft in brand new files says 2008, 2009, 2010? (I see tests/background.py was updated on this ticket to use 2008-2015 AURA/LSST) python/lsst/afw/math/background.py: *I like that there is a section for backwards compatibility here. Will we be supporting these new property set keys when they change in the future? line 171/172. ApproxOrderX/Y defaults when not useApprox: Please add a comment as to why is one default is -1 and the other +1. 809/820 commented-out print statements ip_diffim : look good; no comments.
            Hide
            lauren Lauren MacArthur added a comment -

            Thanks Yusra AlSayyad

            RE consistency between useApprox & usePolynomial: I totally agree, but I think that can be addressed in DM-2920 (for which we will be consulting you and Simon Krughoff on!).

            RE interpolation on doubles: I think this is indeed the case for now. Here are the comments Steve Bickerton [X] wrote in the HSC-1047 ticket:

            – Background was templated over double,float,int; but Approximate was only using float. My first effort was to add double and int to Approximate, but this proved difficult and I could never quite get the explicit instantiations to behave. However, since we don't actually use double or int Backgrounds, it seemed sensible to just stop instantiating them and move on.

            I think his last statement is still valid here, and there don't seem to be any noticeable effects (e.g. lsst_dm_stack_demo passes). If you're not worried, I'm not either

            RE supporting property keys: I don't know, but that will certainly be part of the discussions re DM-2920.

            I went ahead and updated the copyright on every file touched in this this issue and addressed all other comments. I've rebased onto master, re-pushed the branches, and kicked off another Buildbot. Are you ok with me pushing to master if that passes?

            Show
            lauren Lauren MacArthur added a comment - Thanks Yusra AlSayyad RE consistency between useApprox & usePolynomial : I totally agree, but I think that can be addressed in DM-2920 (for which we will be consulting you and Simon Krughoff on!). RE interpolation on doubles : I think this is indeed the case for now. Here are the comments Steve Bickerton [X] wrote in the HSC-1047 ticket: – Background was templated over double,float,int; but Approximate was only using float. My first effort was to add double and int to Approximate, but this proved difficult and I could never quite get the explicit instantiations to behave. However, since we don't actually use double or int Backgrounds, it seemed sensible to just stop instantiating them and move on. I think his last statement is still valid here, and there don't seem to be any noticeable effects (e.g. lsst_dm_stack_demo passes). If you're not worried, I'm not either RE supporting property keys : I don't know, but that will certainly be part of the discussions re DM-2920 . I went ahead and updated the copyright on every file touched in this this issue and addressed all other comments. I've rebased onto master, re-pushed the branches, and kicked off another Buildbot. Are you ok with me pushing to master if that passes?
            lauren Lauren MacArthur made changes -
            Resolution Done [ 10000 ]
            Status In Review [ 10004 ] Done [ 10002 ]
            Hide
            swinbank John Swinbank added a comment -

            Lauren MacArthur – when you get chance, could you add a few words describing what's been done here to the release notes at https://confluence.lsstcorp.org/display/DM/Data+Release+Production+WIP+S15+release+notes? Thanks!

            Show
            swinbank John Swinbank added a comment - Lauren MacArthur – when you get chance, could you add a few words describing what's been done here to the release notes at https://confluence.lsstcorp.org/display/DM/Data+Release+Production+WIP+S15+release+notes? Thanks!
            jbosch Jim Bosch made changes -
            Labels HSC
            swinbank John Swinbank made changes -
            Labels HSC
            krzys Krzysztof Findeisen made changes -
            Link This issue relates to DM-22539 [ DM-22539 ]

              People

              Assignee:
              lauren Lauren MacArthur
              Reporter:
              lauren Lauren MacArthur
              Reviewers:
              Yusra AlSayyad
              Watchers:
              Jim Bosch, John Swinbank, Lauren MacArthur, Paul Price, Simon Krughoff, Yusra AlSayyad
              Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  Jenkins

                  No builds found.