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

Fix position of psf computation for base_SdssShape_psf

    Details

    • Type: Bug
    • Status: Done
    • Priority: Major
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: meas_base
    • Labels:
      None
    • Templates:
    • Story Points:
      1
    • Epic Link:
    • Sprint:
      DRP S17-1
    • Team:
      Data Release Production

      Description

      There is an error in the psf computation of base_SdssShape_psf in meas_base's src/SdssShape.cc in that it does not provide the position of the source, so is just getting the measurement at the position returned by afw's getAveragePosition() in src/detection/Psf.cc for all sources. Please fix.

        Activity

        Hide
        lauren Lauren MacArthur added a comment -

        John Swinbank, could you have a look? I've attached some before and after fix plots and have confirmed that the catalog is now computing the psf shapes at the source position rather than just the image average position for each source:

        before:

        In [10]: srcBefore["base_SdssShape_psf_xx"]
        Out[10]: 
        array([ 4.35643166,  4.35643166,  4.35643166, ...,  4.35643166, 4.35643166,  4.35643166])
        

        after (for same visit/ccd combo);

        In [12]: srcAfter["base_SdssShape_psf_xx"]
        Out[12]: 
        array([ 4.35188276,  4.35838516,  4.37397443, ...,  4.31416827, 4.31451534,  4.31364129])
        

        Jenkins is happy: https://ci.lsst.codes/job/stack-os-matrix/label=centos-7,python=py2/19409/console

        Show
        lauren Lauren MacArthur added a comment - John Swinbank , could you have a look? I've attached some before and after fix plots and have confirmed that the catalog is now computing the psf shapes at the source position rather than just the image average position for each source: before: In [10]: srcBefore["base_SdssShape_psf_xx"] Out[10]: array([ 4.35643166, 4.35643166, 4.35643166, ..., 4.35643166, 4.35643166, 4.35643166]) after (for same visit/ccd combo); In [12]: srcAfter["base_SdssShape_psf_xx"] Out[12]: array([ 4.35188276, 4.35838516, 4.37397443, ..., 4.31416827, 4.31451534, 4.31364129]) Jenkins is happy: https://ci.lsst.codes/job/stack-os-matrix/label=centos-7,python=py2/19409/console
        Hide
        swinbank John Swinbank added a comment -

        Plots look nice. Code change seems to be unambiguously the right thing. Well found.

        But... (& please don't hate me for this) any chance of dropping in a test case to make sure this doesn't happen again? I've not looked at the SdssShape tests to see how straightforward it would be.

        Show
        swinbank John Swinbank added a comment - Plots look nice. Code change seems to be unambiguously the right thing. Well found. But... (& please don't hate me for this) any chance of dropping in a test case to make sure this doesn't happen again? I've not looked at the SdssShape tests to see how straightforward it would be.
        Hide
        lauren Lauren MacArthur added a comment -

        I don't hate you at all, and in fact already looked into it. It's not entirely straight-forward to test for this the way we're currently using testSdssShape.py to test the basic functionality as the test image built is almost as simplistic as it gets, only having one psf model for the entire image (so you'd get the same psf shape at any position of the image). I'll have a closer look to see if it can be adapted...

        Show
        lauren Lauren MacArthur added a comment - I don't hate you at all, and in fact already looked into it. It's not entirely straight-forward to test for this the way we're currently using testSdssShape.py to test the basic functionality as the test image built is almost as simplistic as it gets, only having one psf model for the entire image (so you'd get the same psf shape at any position of the image). I'll have a closer look to see if it can be adapted...
        Hide
        lauren Lauren MacArthur added a comment -

        Ok, many thanks to Yusra AlSayyad for showing me how to create a variable psf model that could be applied to an exposure so that we could put in a unittest for this issue. The only hitch is that it requires use of meas_algorithms's PcaPsf, so the test can't live in meas_base. I've added tests/testSdssShapePsf.py to meas_algorithms. Below I show a printout for the measurements at 3 different source locations to ensure the psf is actually varying:

        source position:  50.1 49.8
             psfTruth =  (ixx=3.89667634765, iyy=3.89667634765, ixy=-0.0)
        sdssShape_psf =  (ixx=3.89668177791, iyy=3.89668177791, ixy=-0.0)
        source position:  -11.6 -1.7
             psfTruth =  (ixx=2.97828641118, iyy=2.97828641118, ixy=-0.0)
        sdssShape_psf =  (ixx=2.978288526, iyy=2.978288526, ixy=-0.0)
        source position:  149.9 50.3
             psfTruth =  (ixx=8.99930898191, iyy=8.99930898191, ixy=0.0)
        sdssShape_psf =  (ixx=8.99930656829, iyy=8.99930656829, ixy=0.0)
        

        I also added a note to tests/testSdssShape.py in meas_base noting this.

        Jenkins is down, but the test passes on my local build. Feel free to have a look when you have time and I'll let you know when I get the Jenkins run going.

        Show
        lauren Lauren MacArthur added a comment - Ok, many thanks to Yusra AlSayyad for showing me how to create a variable psf model that could be applied to an exposure so that we could put in a unittest for this issue. The only hitch is that it requires use of meas_algorithms 's PcaPsf , so the test can't live in meas_base . I've added tests/testSdssShapePsf.py to meas_algorithms . Below I show a printout for the measurements at 3 different source locations to ensure the psf is actually varying: source position: 50.1 49.8 psfTruth = (ixx=3.89667634765, iyy=3.89667634765, ixy=-0.0) sdssShape_psf = (ixx=3.89668177791, iyy=3.89668177791, ixy=-0.0) source position: -11.6 -1.7 psfTruth = (ixx=2.97828641118, iyy=2.97828641118, ixy=-0.0) sdssShape_psf = (ixx=2.978288526, iyy=2.978288526, ixy=-0.0) source position: 149.9 50.3 psfTruth = (ixx=8.99930898191, iyy=8.99930898191, ixy=0.0) sdssShape_psf = (ixx=8.99930656829, iyy=8.99930656829, ixy=0.0) I also added a note to tests/testSdssShape.py in meas_base noting this. Jenkins is down, but the test passes on my local build. Feel free to have a look when you have time and I'll let you know when I get the Jenkins run going.
        Show
        lauren Lauren MacArthur added a comment - Jenkins is happy: https://ci.lsst.codes/job/stack-os-matrix/label=centos-7,python=py2/19428/console
        Hide
        swinbank John Swinbank added a comment -

        Added a fairly trivial comment to the PR on meas_algorithms. Otherwise, ok to merge.

        By the way, the current standards say you should create your own PRs for review. See the discussion here. Not a big deal, obviously!

        Happy holidays.

        Show
        swinbank John Swinbank added a comment - Added a fairly trivial comment to the PR on meas_algorithms. Otherwise, ok to merge. By the way, the current standards say you should create your own PRs for review. See the discussion here . Not a big deal, obviously! Happy holidays.
        Hide
        lauren Lauren MacArthur added a comment -

        Thanks John Swinbank. I added the extra checks, reran Jenkins, and merged to master.

        Show
        lauren Lauren MacArthur added a comment - Thanks John Swinbank . I added the extra checks, reran Jenkins, and merged to master.

          People

          • Assignee:
            lauren Lauren MacArthur
            Reporter:
            lauren Lauren MacArthur
            Reviewers:
            John Swinbank
            Watchers:
            John Swinbank, Lauren MacArthur
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development

                Agile